Skip to content
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

Merged
merged 2 commits into from May 13, 2024

Conversation

kamalcph
Copy link
Collaborator

@kamalcph kamalcph commented May 9, 2024

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)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@kamalcph kamalcph added the tiered-storage Pull requests associated with KIP-405 (Tiered Storage) label May 9, 2024
Copy link
Contributor

@chia7712 chia7712 left a 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 {
Copy link
Contributor

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.

Copy link
Contributor

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 :)

Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks.

Copy link
Member

@satishd satishd left a 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.

@chia7712
Copy link
Contributor

QA is re-triggered, and I will merge it if QA does not show the related error.

@clolov
Copy link
Collaborator

clolov commented May 10, 2024

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)

@kamalcph
Copy link
Collaborator Author

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 boundPort method. If it does not work for you, Could you please open a draft PR? I'll take a look.

@chia7712
Copy link
Contributor

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)

@clolov Could you take a look at #15917 ? I try to use another way to write tests for storage module. ClusterInstace get created and port is bound in testing phase. Maybe that can fix problem you described.

@clolov
Copy link
Collaborator

clolov commented May 13, 2024

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 :)

@chia7712
Copy link
Contributor

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 ClusterInstance works for more test cases.

@chia7712 chia7712 merged commit 576facf into apache:trunk May 13, 2024
1 check failed
@kamalcph kamalcph deleted the storage-test-cleanup branch May 13, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tiered-storage Pull requests associated with KIP-405 (Tiered Storage)
Projects
None yet
5 participants