mingyen066 commented on code in PR #22977:
URL: https://github.com/apache/kafka/pull/22977#discussion_r3674427516
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -399,6 +399,7 @@ RemoteStorageManager createRemoteStorageManager() {
});
}
+ @SuppressWarnings("removal") // revisit when broker.id is removed from
CONFIG_DEF in 5.0 (KIP-1232)
private Plugin<RemoteStorageManager>
configAndWrapRsmPlugin(RemoteStorageManager rsm) {
final Map<String, Object> rsmProps = new
HashMap<>(rlmConfig.remoteStorageManagerProps());
rsmProps.put(ServerConfigs.BROKER_ID_CONFIG, brokerId);
Review Comment:
Thanks for the review, and good catch on this one!
Renaming the key would break plugins that read `broker.id` from
`configure(Map)` — the default RLMM
(`TopicBasedRemoteLogMetadataManagerConfig`) and `LocalTieredStorage` both do,
and would silently get `null`. The same change was actually attempted and
reverted in #18478 for this reason.
That said, your question made me realise we can do better than just leaving
it as is. I've pushed a commit that passes `node.id` alongside `broker.id`,
with Kafka's own plugins now reading `node.id`. `broker.id` stays for
compatibility and will stop being passed in 5.0, and I'll update the KIP to
cover this.
One small note in case it's useful: `storage` doesn't depend on `raft`, so
`KRaftConfigs.NODE_ID_CONFIG` isn't reachable from here — hence the local
constants.
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
Review Comment:
Same here — both plugins now receive `node.id` as well. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
