ShivsundarR commented on PR #19886:
URL: https://github.com/apache/kafka/pull/19886#issuecomment-3546745726
Thanks @kirktrue for the review,
I have added a couple of integration tests to test both share-consumers and
regular consumers.
> the logic in this PR is assuming the UnsentRequest represents a
FIND_COORDINATOR RPC because it has an empty Node.
Yes that's true in a way but I was thinking the `u.node.isEmpty()` check
represents the general idea of not issuing requests to an unknown node when we
are closing the network thread, in future if we modify this to have other APIs,
the idea will still work.
But yes as of now both ways work, I was thinking to retain the current code
to make it behaviour specific, what do you think?
> I had another minor concern regarding the handling in
CoordinatorRequestManager of the NetworkException that's passed to onFailure().
How does the CoordinatorRequestManager logic handle that error?
For example, does this result in potentially misleading logging?
No, this does not lead to misleading logging, it does not enter the
"Rediscovery" part in `markCoordinatorUnknown` as the coordinator is `null`.
Following are the debug logs I got when this the request is completed.
```
DEBUG [ShareConsumer clientId=consumer-group1-2, groupId=group1] Removing
unsent request
UnsentRequest{requestBuilder=FindCoordinatorRequestData(key='group1',
keyType=0, coordinatorKeys=[]),
handler=org.apache.kafka.clients.consumer.internals.NetworkClientDelegate$FutureCompletionHandler@32e301da,
node=Optional.empty, remainingMs=29251} because the client is closing
(org.apache.kafka.clients.consumer.internals.NetworkClientDelegate:246)
DEBUG [ShareConsumer clientId=consumer-group1-2, groupId=group1]
FindCoordinator request failed due to retriable exception
(org.apache.kafka.clients.consumer.internals.CoordinatorRequestManager:205)
org.apache.kafka.common.errors.NetworkException: The server disconnected
before a response was received.
```
We just log that `findCoordinator` failed with a retriable exception and
print the exception as well. So it should be fine I guess.
--
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]
