lucasbru commented on code in PR #22664:
URL: https://github.com/apache/kafka/pull/22664#discussion_r3469611124
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/StreamsGroupTopologyDescriptionManager.java:
##########
@@ -73,6 +74,7 @@ public class StreamsGroupTopologyDescriptionManager
implements AutoCloseable {
private final Logger log;
private final Optional<StreamsGroupTopologyDescriptionPlugin> plugin;
private final StreamsGroupTopologyDescriptionBackoff backoff;
+ private final GroupCoordinatorMetrics metrics;
Review Comment:
Not sure I buy the second part - we could equally move the set recording to
the GroupMetricsManager. But I guess it's fine, since we also record other
metrics here.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/StreamsGroupTopologyDescriptionManager.java:
##########
@@ -533,24 +539,40 @@ private void applyGetTopologyOutcome(
Throwable cause = Errors.maybeUnwrapException(throwable);
log.warn("Topology description plugin getTopology failed for group
{}.",
describedGroup.groupId(), cause != null ? cause : throwable);
+ recordGetError();
describedGroup.setTopologyDescriptionStatus(TOPOLOGY_DESCRIPTION_STATUS_ERROR);
return;
}
+ // The plugin call itself completed normally. A null return is the
documented
+ // "plugin no longer has the data" path and surfaces as NOT_STORED,
not an error,
+ // so it still counts as a successful getTopology.
if (topology == null) {
+ recordGetSuccess();
describedGroup.setTopologyDescriptionStatus(TOPOLOGY_DESCRIPTION_STATUS_NOT_STORED);
return;
}
try {
describedGroup.setTopologyDescription(
StreamsGroupTopologyDescriptionConverter.toDescribeResponse(topology));
describedGroup.setTopologyDescriptionStatus(TOPOLOGY_DESCRIPTION_STATUS_AVAILABLE);
+ recordGetSuccess();
} catch (Exception conversionError) {
- // Defensive catch, should be unreachable in practice
+ // Defensive catch, should be unreachable in practice. The outcome
surfaces as
+ // ERROR to the client, so count it as a failed getTopology to
stay consistent.
+ recordGetError();
describedGroup.setTopologyDescription(null);
describedGroup.setTopologyDescriptionStatus(TOPOLOGY_DESCRIPTION_STATUS_ERROR);
}
}
+ private void recordGetSuccess() {
Review Comment:
I'd just inline the one-liners.
--
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]
