lianetm commented on code in PR #17989:
URL: https://github.com/apache/kafka/pull/17989#discussion_r1868073149
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractHeartbeatRequestManager.java:
##########
@@ -382,8 +382,16 @@ private void onErrorResponse(final R response, final long
currentTimeMs) {
break;
case UNSUPPORTED_VERSION:
- message = "The cluster doesn't yet support the new consumer
group protocol." +
- " Set group.protocol=classic to revert to the classic
protocol until the cluster is upgraded.";
+ if (errorMessage == null) {
+ message = "The cluster does not support the new consumer
group protocol. " +
+ "Set group.protocol=classic on the consumer configs to
revert to " +
+ "the classic protocol until the cluster is upgraded.";
+ } else {
+ // Keep custom message present in the error. This could be
the case where
+ // HB version required for a feature being used is not
available
+ // (ex. regex requires HB v>0), or any other custom
message included in the response.
+ message = errorMessage;
+ }
Review Comment:
you're right:
1. If the request fails to build on the client, it lands directly into the
`onFailure` because the response is created with the `UnsupportedVersion`
exception
https://github.com/apache/kafka/blob/a8cdbaf4b30a119c66a9a85f6cab32610cd17fa9/clients/src/main/java/org/apache/kafka/clients/NetworkClient.java#L588-L590
(meaning that the custom msg for v1 required for regex goes out directly,
nothing extra needed)
2. the custom msg for protocol not supported that was here won't actually
come out if the request fails locally because the client will fail when
checking the supported apis, and go directly to `onFailure` again, with this:
https://github.com/apache/kafka/blob/a8cdbaf4b30a119c66a9a85f6cab32610cd17fa9/clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java#L157
(We still need to keep the msg here to make sure that it shows in the case
where the broker does support the HB api but the new protocol is not enabled)
So, I made the changes for both. I added different msgs for the case where
protocol is not supported (old broker), vs protocol not enabled, thoughts?
##########
clients/src/main/java/org/apache/kafka/common/requests/ConsumerGroupHeartbeatRequest.java:
##########
@@ -59,6 +60,11 @@ public Builder(ConsumerGroupHeartbeatRequestData data,
boolean enableUnstableLas
@Override
public ConsumerGroupHeartbeatRequest build(short version) {
+ if (version == 0 && data.subscribedTopicRegex() != null) {
+ throw new UnsupportedVersionException("The cluster does not
support regular expressions resolution " +
+ "on version " + version + ". It must be upgraded to
version >= 1 to allow to subscribe to a " +
+ "SubscriptionPattern.");
Review Comment:
makes sense , done
--
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]
