New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-16696: Removed the in-memory implementation of RSM and RLMM #15911
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamalcph nice cleanup. one idea is left. PATL
private RemoteLogMetadataManager remoteLogMetadataManager = new TopicBasedRemoteLogMetadataManagerWrapperWithHarness(); | ||
|
||
@Test | ||
public void testFetchSegments() throws Exception { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me TopicBasedRemoteLogMetadataManagerWrapperWithHarness
can be removed also. We don't use the wrapper actually. This test can be modified by following style:
@Test
public void testFetchSegments() throws Exception {
try (TopicBasedRemoteLogMetadataManagerHarness remoteLogMetadataManagerHarness = new TopicBasedRemoteLogMetadataManagerHarness()) {
RemoteLogMetadataManager remoteLogMetadataManager = remoteLogMetadataManagerHarness.remoteLogMetadataManager();
noted TopicBasedRemoteLogMetadataManagerHarness
needs to implement AutoClosable
, and remoteLogMetadataManager
should be a public method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be addressed as follow-up I think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kamalcph for the cleanup. Inmemory implementations were added fro UTs in the early days while RSM and RLMM interfaces were added. Those are no more needed as we can use LocalTieredStorage
for the same. LGTM.
QA is re-triggered, and I will merge it if QA does not show the related error. |
Can you actually not remove this because I was starting to use it in the GetOffsetShellToolTest? The problem I faced with using the topic-based RLMM is that it requires the bootstrap-servers to initialise correctly. However, said bootstrap-servers are not present at initialisation time (due to determining the port dynamically) |
We can get the port from the Kafkabroker using the |
@clolov Could you take a look at #15917 ? I try to use another way to write tests for storage module. |
Okay, let's go forward with this so that you are not blocked on me, I will try to figure out a way forward and if not I will write again :) |
@clolov thanks! Please feel free to ping me if you have a draft PR. I'd like to make |
The in-memory implementation of RSM and RLMM were written to write the unit/integration tests: #10218
This is not used by any of the tests and superseded by the LocalTieredStorage framework which uses local-disk as secondary storage and topic as RLMM. Using the LocalTieredStorage framework is the preferred way to write the integration tests to capture any regression as it uses the internal topic as storage for RLMM which is the default implementation.
Committer Checklist (excluded from commit message)