Skip to content

KAFKA-16696: Removed the in-memory implementation of RSM and RLMM#15911

Merged
chia7712 merged 2 commits intoapache:trunkfrom
kamalcph:storage-test-cleanup
May 13, 2024
Merged

KAFKA-16696: Removed the in-memory implementation of RSM and RLMM#15911
chia7712 merged 2 commits intoapache:trunkfrom
kamalcph:storage-test-cleanup

Conversation

@kamalcph
Copy link
Copy Markdown
Contributor

@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 requested review from chia7712, satishd and showuon May 9, 2024 17:44
@kamalcph kamalcph added the tiered-storage Related to the Tiered Storage feature label May 9, 2024
Copy link
Copy Markdown
Member

@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


@Test
public void testFetchSegments() throws Exception {
try {
Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
Member

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

@clolov
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Member

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
Copy Markdown
Contributor

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
Copy Markdown
Member

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
@kamalcph kamalcph deleted the storage-test-cleanup branch May 13, 2024 15:42
TaiJuWu pushed a commit to TaiJuWu/kafka that referenced this pull request Jun 8, 2024
…che#15911)

Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
gongxuanzhang pushed a commit to gongxuanzhang/kafka that referenced this pull request Jun 12, 2024
…che#15911)

Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tiered-storage Related to the Tiered Storage feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants