dajac commented on code in PR #19856:
URL: https://github.com/apache/kafka/pull/19856#discussion_r2123327253
##########
clients/src/main/java/org/apache/kafka/clients/GroupRebalanceConfig.java:
##########
@@ -53,8 +54,15 @@ public GroupRebalanceConfig(AbstractConfig config,
ProtocolType protocolType) {
// Consumer and Connect use different config names for defining
rebalance timeout
if ((protocolType == ProtocolType.CONSUMER) || (protocolType ==
ProtocolType.SHARE)) {
this.rebalanceTimeoutMs =
config.getInt(CommonClientConfigs.MAX_POLL_INTERVAL_MS_CONFIG);
+ // Connect doesn't support rack id.
+ // The GroupCoordinatorService throws error if the rackId is
empty. The default value of client.rack is empty string.
+ // Skip empty rackId to avoid InvalidRequestException.
+ this.rackId =
config.getString(CommonClientConfigs.CLIENT_RACK_CONFIG).isEmpty() ?
+ Optional.empty() :
+
Optional.of(config.getString(CommonClientConfigs.CLIENT_RACK_CONFIG));
Review Comment:
In `ConsumerCoordinator`, we have the following code:
```
this.rackId = rackId == null || rackId.isEmpty() ? Optional.empty() :
Optional.of(rackId);
```
Hence, we can use the same logic for the classic protocol. I wonder if we
could just do the following here for all protocols.
```
String rackId = config.getString(CommonClientConfigs.CLIENT_RACK_CONFIG);
this.rackId = rackId == null || rackId.isEmpty() ? Optional.empty() :
Optional.of(rackId);
```
We may also want to update `ConsumerCoordinator` to get it from
`GroupRebalanceConfig` too.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManager.java:
##########
@@ -306,6 +307,13 @@ public ConsumerGroupHeartbeatRequestData
buildRequestData() {
sentFields.localAssignment = local;
}
+ // RackId - sent when joining or if it has changed since the last
heartbeat
+ String rackId = membershipManager.rackId().orElse(null);
+ if (sendAllFields || !Objects.equals(rackId, sentFields.rackId)) {
+ data.setRackId(rackId);
+ sentFields.rackId = rackId;
+ }
Review Comment:
Could we also test this?
--
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]
