lianetm commented on code in PR #18101:
URL: https://github.com/apache/kafka/pull/18101#discussion_r1884366119
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ShareHeartbeatRequestManagerTest.java:
##########
@@ -422,6 +425,29 @@ public void testHeartbeatResponseOnErrorHandling(final
Errors error, final boole
}
}
+ @ParameterizedTest
+ @ValueSource(strings = {SHARE_PROTOCOL_NOT_SUPPORTED_MSG})
+ public void testUnsupportedVersion(String errorMsg) {
Review Comment:
this is truly `testUnsupportedVersionGeneratedOnTheClient` (client response
with exception). Renaming will maybe clarify, and we should ensure we have
coverabe for something like `testUnsupportedVersionGeneratedOnTheBroker`
(client response with `ErrorCode`), is it already covered?
The outcome of both tests should be the same, but the execute diff code
paths modified in this PR (handleSpecificError and
handleSpecificFailure/exception)
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManager.java:
##########
@@ -97,6 +99,18 @@ public boolean handleSpecificError(final
ConsumerGroupHeartbeatResponse response
boolean errorHandled;
switch (error) {
+ case UNSUPPORTED_VERSION:
+ // Handle consumer-specific unsupported version error
+ String message = CONSUMER_PROTOCOL_NOT_SUPPORTED_MSG;
+ if (errorMessage.contains(REGEX_RESOLUTION_NOT_SUPPORTED_MSG))
{
+ // If the error is about regex subscription, use the
original error message
+ message = errorMessage;
+ }
Review Comment:
Yes. The implementation of that
onSpecificFailure/handleSpecificExceptionInResponse on each consumer should
cover this bit I expect, with the correct msg:
https://github.com/apache/kafka/blob/497f50048305733606769a688b940cadce98521f/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractHeartbeatRequestManager.java#L322-L325
And in the AbstractHBMgr onFailure probably just need something like:
```
if (exception instanceof RetriableException) {
...
} else if (!onSpecificFailure()) {
handleFatalFailure(exception);
}
```
We don't need any specifics for the regex case I would expect. It should
land on the handleFatalFailure and propagate the msg it has (it will only ever
be generated when building consumer HB, so no risk for the share consumer)
With that seems we cover the specifics of handling responses with exceptions
(generated on the client) , and in a very similar way to how we handle
responses with error codes (generated on the broker).
--
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]
