squah-confluent commented on code in PR #15559:
URL: https://github.com/apache/kafka/pull/15559#discussion_r1534709657
##########
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala:
##########
@@ -921,7 +921,10 @@ private[group] class GroupCoordinator(
): Unit = {
val (error, verificationGuard) = errorAndGuard
if (error != Errors.NONE) {
- val finalError =
GroupMetadataManager.maybeConvertOffsetCommitError(error)
+ val finalError = error match {
+ case Errors.NETWORK_EXCEPTION =>
Errors.COORDINATOR_LOAD_IN_PROGRESS
Review Comment:
Thank you for the review!
`maybeConvertOffsetCommitError` is also used in
[`GroupMetadataManager.createPutCacheCallback`](https://github.com/apache/kafka/blob/7e85d7d32e8cb4f3fa9b01fd4197ee43d64ba6d0/core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala#L430).
I wasn't sure whether it made sense to translate network exceptions on that
path too. I'm happy to move the translation there if you think it makes sense.
I have the same uncertainty about
[`handleOperationException`](https://github.com/apache/kafka/blob/7e85d7d32e8cb4f3fa9b01fd4197ee43d64ba6d0/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java#L1084)
in the new group coordinator. All the other group coordinator methods use it
to translate errors and I was not sure if it made sense to translate network
exceptions there.
Also, I agree, a comment to explain the choice of error would be good. I'll
add one here and in ReplicaManager too.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
