KAFKA-18628: Deprecate broker.id config (KIP-1232) - #22977
Conversation
gaurav-narula
left a comment
There was a problem hiding this comment.
Thanks for the PR! Mostly good, a few comments
…smatch error, parameterise test against doLog
gaurav-narula
left a comment
There was a problem hiding this comment.
Thanks for the updates. LGTM!
| * When clients connect to the cluster, they now include cluster and node information to enable detection and handling of misrouted connections. For further details, please refer to [KIP-1242](https://cwiki.apache.org/confluence/x/W4LMFw). | ||
| * The `kafka-cluster.sh` tool now provides an `api-versions` command to display the API versions supported by the brokers or controllers, and it accepts both `--bootstrap-server` and `--bootstrap-controller`. As a result, `kafka-broker-api-versions.sh` is deprecated and will be removed in the next major release; use `kafka-cluster.sh api-versions` instead. For further details, please refer to [KIP-1220](https://cwiki.apache.org/confluence/x/-QkbFw). | ||
| * The `broker.id` configuration is deprecated and will be removed in Kafka 5.0. Please use `node.id` instead. For further details, please refer to [KIP-1232](https://cwiki.apache.org/confluence/x/Hgp3Fw). | ||
| * Tiered storage plugins are now configured with `node.id` in addition to `broker.id`. Since `broker.id` will no longer be passed to `RemoteStorageManager` and `RemoteLogMetadataManager` implementations in Kafka 5.0, plugins should read `node.id` instead. For further details, please refer to [KIP-1232](https://cwiki.apache.org/confluence/x/Hgp3Fw). |
There was a problem hiding this comment.
Could you also update the KIP to reflect this change?
There was a problem hiding this comment.
Thanks for reviewing! I've updated the KIP to cover the tiered storage plugin config: node.id is passed alongside broker.id in 4.4, and broker.id is dropped in 5.0.
| * | ||
| * @deprecated Use {@link #NODE_ID} instead. This key is no longer passed to plugins from Kafka 5.0 (KIP-1232). | ||
| */ | ||
| @Deprecated(since = "4.4", forRemoval = true) |
There was a problem hiding this comment.
Could you explain why the test file needs @Deprecated?
There was a problem hiding this comment.
Good catch. removed.
| */ | ||
| @Deprecated(since = "4.4", forRemoval = true) | ||
| public static final String BROKER_ID = "broker.id"; | ||
| public static final String NODE_ID = "node.id"; |
There was a problem hiding this comment.
Could you also update the KIP to reflect this change?
There was a problem hiding this comment.
Covered by the KIP update as well — TopicBasedRemoteLogMetadataManagerConfig.BROKER_ID is deprecated in 4.4 and removed in 5.0.
| initializationRetryIntervalMs = (long) parsedConfigs.get(REMOTE_LOG_METADATA_INITIALIZATION_RETRY_INTERVAL_MS_PROP); | ||
| initializationRetryMaxTimeoutMs = (long) parsedConfigs.get(REMOTE_LOG_METADATA_INITIALIZATION_RETRY_MAX_TIMEOUT_MS_PROP); | ||
| clientIdPrefix = REMOTE_LOG_METADATA_CLIENT_PREFIX + "_" + props.get(BROKER_ID); | ||
| clientIdPrefix = REMOTE_LOG_METADATA_CLIENT_PREFIX + "_" + props.get(NODE_ID); |
There was a problem hiding this comment.
why don't we handle the deprecation?
Object nodeIdObj = props.get(NODE_ID);
if (nodeIdObj == null) {
nodeIdObj = props.get(BROKER_ID);
if (nodeIdObj == null) {
throw new ConfigException("Both '" + NODE_ID + "' and '" + BROKER_ID + "' are missing. Please configure '" + NODE_ID + "'.");
}
LOG.warn("The '{}' configuration in remote log metadata manager is deprecated and will be removed in Kafka 5.0. " +
"Please use '{}' instead.", BROKER_ID, NODE_ID);
} else if (props.containsKey(BROKER_ID)) {
LOG.warn("The '{}' configuration is deprecated and will be ignored in Kafka 5.0. " +
"Please use '{}' instead.", BROKER_ID, NODE_ID);
}
clientIdPrefix = REMOTE_LOG_METADATA_CLIENT_PREFIX + "_" + nodeIdObj;
There was a problem hiding this comment.
Done, with ConfigException. I left out the else if (props.containsKey(BROKER_ID)) branch — RemoteLogManager passes both keys unconditionally, so it would warn on every startup about a key the operator didn't set.
| final String transfererClass = (String) configs.get(TRANSFERER_CLASS_CONFIG); | ||
| final String isDeleteEnabled = (String) configs.get(ENABLE_DELETE_API_CONFIG); | ||
| final Integer brokerIdInt = (Integer) configs.get(BROKER_ID); | ||
| final Integer nodeIdInt = (Integer) configs.get(NODE_ID); |
| * The ID of the broker which owns this instance of {@link LocalTieredStorage}. | ||
| * The ID of the node which owns this instance of {@link LocalTieredStorage}. | ||
| */ | ||
| public static final String BROKER_ID = "broker.id"; |
There was a problem hiding this comment.
Should we also deprecate this one in 4.4 and remove only in 5.0? External consumers of Kafka test JAR would get a compile error.
There was a problem hiding this comment.
Restored as deprecated, thanks.
Follow the deprecation cycle rather than switching the key outright: TopicBasedRemoteLogMetadataManagerConfig and LocalTieredStorage read node.id and fall back to broker.id with a warning, so callers that construct them directly keep working through 4.x. Both now fail fast when neither key is present; previously a missing id was silently stringified into the client id prefix. Restore LocalTieredStorage.BROKER_ID as deprecated instead of removing it, since the test-fixtures jar is published and external code may compile against it. Document node.id in the RemoteStorageManager and RemoteLogMetadataManager javadoc, which is the only written form of the plugin contract.
chia7712
left a comment
There was a problem hiding this comment.
LGTM, and the remaining issues could be fixed by the follow-up PR
| throw new InvalidConfigurationException( | ||
| "Broker ID is required to configure the LocalTieredStorage manager."); | ||
| if (nodeIdInt == null) { | ||
| throw new InvalidConfigurationException(format( |
There was a problem hiding this comment.
This check could be moved to nodeId, and then nodeId could return int type instead of Integer
Implement KIP-1232.
broker.idconfiguration for removal in Kafka 5.0.broker.idisexplicitly set, pointing users to
node.id.node.idto the tiered storage plugins alongsidebroker.id,and switch Kafka's own
RemoteStorageManagerandRemoteLogMetadataManagerimplementations to read it.broker.idisstill passed for compatibility and will be dropped in 5.0.
node.id/broker.idmismatch error so it points atremoving
broker.idrather than keeping both in sync.broker.idsettings in test files —node.idalone is enough via the existing synonym mechanism.
The startup warning is checked on the raw properties in
KafkaConfig.fromProps, becausepopulateSynonymscopiesnode.idinto
broker.id, sooriginals()cannot tell whether the user actuallyset it. It is not guarded by
doLog, since the broker startup pathcalls
fromPropswithdoLog = false.The tiered storage plugin change is not covered by the KIP yet — I will
amend KIP-1232 to include it.
Test result:
Reviewers: Gaurav Narula gaurav_narula2@apple.com, Ken Huang
s7133700@gmail.com, Chia-Ping Tsai chia7712@gmail.com, Federico
Valeri fedevaleri@gmail.com