KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager #19286

TaiJuWu · 2025-03-25T20:58:45Z

Remove RemoteLogManager#startup and
RemoteLogManager#onEndpointCreated
Move endpoint creation to BrokerServer
Move RemoteLogMetadataManager#configure and
RemoteLogStorageManager#configure to RemoteLogManager constructor

Reviewers: Mickael Maison [email protected], Ken Huang
[email protected], Jhen-Yung Hsu [email protected]

This reverts commit 86487b2.

This reverts commit 08c1b2a.

fix test

mimaison

Thanks for the PR. It's really not nice that we need to build the plugin in 2 stages. I left a suggestion to refactor the RemoteLogManager creation logic. Let's see what people think

core/src/main/java/kafka/log/remote/RemoteLogManager.java

TaiJuWu · 2025-04-03T22:50:44Z

Thanks for the PR! Can we also add an integration test? For example something like I'm doing in https://github.com/apache/kafka/pull/19050/files#diff-6ecc777f37395e25591a118f0eef88d602c765a66adace6247f98cf4ea159bc9

I will take a look later.

server/src/test/java/org/apache/kafka/server/MonitorablePluginsIntegrationTest.java

core/src/main/scala/kafka/server/BrokerServer.scala

TaiJuWu · 2025-04-10T23:00:08Z

Thanks for the PR! Can we also add an integration test? For example something like I'm doing in https://github.com/apache/kafka/pull/19050/files#diff-6ecc777f37395e25591a118f0eef88d602c765a66adace6247f98cf4ea159bc9

I have updated integration test, PTAL.

mimaison

Thanks for the updates, I made another pass and left a few more comments

mimaison · 2025-04-14T14:54:09Z

core/src/main/java/kafka/log/remote/RemoteLogManager.java

+
+        remoteStorageManagerPlugin = configAndWrapRSMPlugin(createRemoteStorageManager());
+        remoteLogMetadataManagerPlugin = configAndWrapRLMMPlugin(createRemoteLogMetadataManager());
+        remoteLogManagerConfigured = true;


Do we need to keep this field? It's always set to true in the constructor, so it can never be null if you have an instance of RemoteLogManager

mimaison · 2025-04-14T15:12:43Z

core/src/main/java/kafka/log/remote/RemoteLogManager.java

@@ -237,9 +239,12 @@ public RemoteLogManager(RemoteLogManagerConfig rlmConfig,
        this.updateRemoteLogStartOffset = updateRemoteLogStartOffset;
        this.brokerTopicStats = brokerTopicStats;
        this.metrics = metrics;
+        this.endpoint = endpoint;
+
+        remoteStorageManagerPlugin = configAndWrapRSMPlugin(createRemoteStorageManager());


Could we merge the create and config methods? For example have createRemoteStorageManagerPlugin() do it all and return Plugin<RemoteStorageManager>.

Same for remoteLogMetadataManagerPlugin

The reason I did not merge this two methods was if we merge them into one, we will include the configuration step when creating RLMM and RSM in test (we use createRemoteStorageManager in test).

That means we will miss real configuration step in test if merge them (configuration step should be included in createRemoteStorageManagerPlugin)

mimaison · 2025-04-14T15:15:51Z

core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java

+            assertEquals(true, ((MonitorableNoOpRemoteStorageManager) remoteLogManager.storageManager()).pluginMetrics);
+            assertEquals(true, ((MonitorableNoOpRemoteLogMetadataManager) remoteLogManager.remoteLogMetadataManager()).pluginMetrics);


We can use assertTrue()

mimaison · 2025-04-14T15:17:41Z

server/src/test/java/org/apache/kafka/server/MonitorablePluginsIntegrationTest.java

+        return tags;
+    }
+
+}


nit: missing new line

mimaison · 2025-04-14T15:17:51Z

server/src/test/java/org/apache/kafka/server/MonitorablePluginsIntegrationTest.java

+        }
+    }
+
+    private static Map<String, String> expectedTags(String config, String clazz) {


Do we really need this method?

Remove it, thanks.

After merging trunk and move private method to the end, this method is back.

mimaison · 2025-04-17T09:12:31Z

Can you rebase on trunk?

TaiJuWu · 2025-04-17T10:19:11Z

Can you rebase on trunk?

Update, thanks.

mimaison

LGTM

…teStorageManager (apache#19286) 1. Remove `RemoteLogManager#startup` and `RemoteLogManager#onEndpointCreated` 2. Move endpoint creation to `BrokerServer` 3. Move `RemoteLogMetadataManager#configure` and `RemoteLogStorageManager#configure` to RemoteLogManager constructor Reviewers: Mickael Maison <[email protected]>, Ken Huang <[email protected]>, Jhen-Yung Hsu <[email protected]>

TaiJuWu added 7 commits March 25, 2025 10:15

implment production code

9761bfe

implement prod code

108129d

fix compiler

c21ec89

Revert "fix compiler"

3f6ca5c

This reverts commit 86487b2.

Revert "implement prod code"

5e0ff34

This reverts commit 08c1b2a.

fix test

0f5c308

add Utils.closeQuietly

5063d0d

github-actions bot added triage PRs from the community core Kafka Broker tiered-storage Related to the Tiered Storage feature small Small PRs labels Mar 25, 2025

add test

da3e54e

TaiJuWu force-pushed the 877_log branch from ada7e5e to da3e54e Compare March 25, 2025 21:35

refactor

5979f30

TaiJuWu changed the title ~~(WIP) KAFKA-18891: Add support for RemoteLogMetadataManager and RemoteStorageManager~~ (WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Mar 26, 2025

merge two variable

3e7f5b2

fix test

github-actions bot added clients and removed small Small PRs labels Mar 26, 2025

TaiJuWu added 3 commits March 26, 2025 05:01

refactor

518f03a

Merge branch 'trunk' into 877_log

63eaea9

improve test

d4ef41d

TaiJuWu changed the title ~~(WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Mar 26, 2025

TaiJuWu changed the title ~~KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ (WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Mar 26, 2025

TaiJuWu changed the title ~~(WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Mar 26, 2025

TaiJuWu requested a review from mimaison March 26, 2025 09:50

mimaison reviewed Mar 26, 2025

View reviewed changes

core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated Show resolved Hide resolved

github-actions bot removed the triage PRs from the community label Mar 27, 2025

TaiJuWu added 3 commits April 1, 2025 10:20

apply Maison's implementation

4f07357

Merge branch 'trunk' into 877_log

7a580bf

rename and small refactor

a9b90d2

address Miason comment

13e8b73

TaiJuWu added 2 commits April 10, 2025 13:39

Merge branch 'trunk' into 877_log

31d52fe

copy file from Miason PR

aa94317

github-actions bot added the build Gradle build or GitHub Actions label Apr 10, 2025

TaiJuWu changed the title ~~KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ (WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Apr 10, 2025

m1a2st reviewed Apr 10, 2025

View reviewed changes

server/src/test/java/org/apache/kafka/server/MonitorablePluginsIntegrationTest.java Outdated Show resolved Hide resolved

add it

f28689e

TaiJuWu force-pushed the 877_log branch from 8ebfc94 to f28689e Compare April 10, 2025 14:40

TaiJuWu changed the title ~~(WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Apr 10, 2025

TaiJuWu changed the title ~~KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ (WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Apr 10, 2025

Yunyung reviewed Apr 10, 2025

View reviewed changes

server/src/test/java/org/apache/kafka/server/MonitorablePluginsIntegrationTest.java Outdated Show resolved Hide resolved

core/src/main/scala/kafka/server/BrokerServer.scala Outdated Show resolved Hide resolved

TaiJuWu changed the title ~~(WIP) KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager~~ KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager Apr 10, 2025

TaiJuWu added 2 commits April 11, 2025 06:59

address Yunyung comments

e77bee1

Merge branch 'trunk' into 877_log

1541865

mimaison reviewed Apr 14, 2025

View reviewed changes

TaiJuWu added 5 commits April 15, 2025 13:40

address maison comments

5c0b221

Merge branch 'trunk' into 877_log

cd5d51b

Merge branch 'trunk' into 877_log

5c1cf46

fix build

f18e1b2

fix test

e3eed4b

TaiJuWu added 2 commits April 17, 2025 17:53

Merge branch 'trunk' into 877_log

fbfa598

fix build

45b6c5d

mimaison approved these changes Apr 18, 2025

View reviewed changes

mimaison merged commit 6e4e0df into apache:trunk Apr 18, 2025
23 checks passed

TaiJuWu deleted the 877_log branch April 18, 2025 13:10

		assertEquals(true, ((MonitorableNoOpRemoteStorageManager) remoteLogManager.storageManager()).pluginMetrics);
		assertEquals(true, ((MonitorableNoOpRemoteLogMetadataManager) remoteLogManager.remoteLogMetadataManager()).pluginMetrics);

KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager #19286

KAFKA-18891: KIP-877 add support for RemoteLogMetadataManager and RemoteStorageManager #19286

Uh oh!

Conversation

TaiJuWu commented Mar 25, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TaiJuWu commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TaiJuWu commented Apr 10, 2025

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Apr 17, 2025

Uh oh!

TaiJuWu commented Apr 17, 2025

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TaiJuWu commented Mar 25, 2025 •

edited by github-actions bot

Loading

TaiJuWu commented Apr 3, 2025 •

edited

Loading

TaiJuWu Apr 15, 2025 •

edited

Loading