C0urante commented on code in PR #13458:
URL: https://github.com/apache/kafka/pull/13458#discussion_r1153964741
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedHerder.java:
##########
@@ -291,6 +291,7 @@ public DistributedHerder(DistributedConfig config,
String clientIdConfig =
config.getString(CommonClientConfigs.CLIENT_ID_CONFIG);
String clientId = clientIdConfig.length() <= 0 ? "connect-" +
CONNECT_CLIENT_ID_SEQUENCE.getAndIncrement() : clientIdConfig;
+ String escapedClientId = clientId.replace("%", "%%");
Review Comment:
🙇 Good eye!
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java:
##########
@@ -200,6 +200,8 @@ public Map<String, String> workerConfig(SourceAndTarget
sourceAndTarget) {
// fill in reasonable defaults
props.putIfAbsent(GROUP_ID_CONFIG, sourceAndTarget.source() + "-mm2");
+ String groupId = props.get(GROUP_ID_CONFIG);
+ props.putIfAbsent(CommonClientConfigs.CLIENT_ID_CONFIG, groupId + "|"
+ sourceAndTarget);
Review Comment:
We already [include the group
ID](https://github.com/apache/kafka/blob/b77b7a6f6f6d39f8064fc5fb11f25b09c58c4228/connect/runtime/src/main/java/org/apache/kafka/connect/util/ConnectUtils.java#L204)
when constructing the base client ID from the `DistributedConfig` that will be
instantiated with these properties; in most cases, it'll be redundant to add it
here.
If it's to cover the case of the shared admin client used for each MM2
herder, we can handle that in [the same
way](https://github.com/apache/kafka/blob/b77b7a6f6f6d39f8064fc5fb11f25b09c58c4228/connect/runtime/src/main/java/org/apache/kafka/connect/cli/ConnectDistributed.java#L77)
that we do in vanilla distributed mode in Kafka Connect.
If it's to cover the client ID used by the `DistributedHerder`, we can
either leave that as-is for now (I'm not sure it's used for any critical
logic/threads/etc. by that class), or implement some way of giving a different
client ID specifically to this class without affecting the return value of
`ConnectUtils::clientIdBase`.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
