aliehsaeedii commented on code in PR #22664:
URL: https://github.com/apache/kafka/pull/22664#discussion_r3468140353
##########
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:
Recording the get sensors here in the manager — rather than in
`GroupCoordinatorService` like set/delete — reads as an asymmetry at first, but
it's actually the right placement. The get disposition (null return ->
NOT_STORED/success, async-error/sync-throw/null-future -> error, converted ->
success) is only finally resolved inside `maybeAttachOne` /
`applyGetTopologyOutcome`; the service never sees that classification.
Recording it here keeps the metric next to the single source of truth instead
of leaking the outcome back out to the service just to count it, which is why
the new `GroupCoordinatorMetrics` dependency belongs on this class. Set is the
mirror image — its outcome is the `PluginOutcome.kind()` dispatched in the
service's `thenCompose`, so recording there is equally natural. Good design.
We can add a one-line comment here noting the deliberate split so the next
reader doesn't mistake it for an inconsistency with set/delete.
--
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]
