dajac commented on code in PR #17989:
URL: https://github.com/apache/kafka/pull/17989#discussion_r1867236561
##########
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:
nit: I wonder whether we should precise that the version is the version of
the ConsumerGroupHeartbeat API. Users may confused it with the version of the
client or the server. What do you think?
##########
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:
As a second thought, I believe that we don't need to change this code at
all. My understanding is that we reach this code only if the server returns a
response containing the UNSUPPORTED_VERSION error. If the request building
fails because the required version of the API is not available, it lands in
`onFailure` directly. Hence, I believe that your custom error goes through
directly.
This also means that the custom message that we use here for when the new
protocol is not available is only provided if the request goes to the server.
If it fails locally, it won't be used at all. Do you confirm?
--
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]
