ableegoldman commented on code in PR #12235:
URL: https://github.com/apache/kafka/pull/12235#discussion_r901061337
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java:
##########
@@ -192,6 +217,28 @@ public <K, V> void send(final String topic,
} else {
log.warn("Received offset={} in produce response for {}",
metadata.offset(), tp);
}
+
+ if (!topic.endsWith("-changelog")) {
+ // we may not have created a sensor yet if the node uses
dynamic topic routing
Review Comment:
~~No, it does refer to the `if` branch -- if
`sinkNodeToProducedSensorByTopic.get` returns null, that means we haven't seen
this topic yet/constructed a sensor for it during initialization.~~
edit: this is wrong, see following comment
I do see how it might be confusing if it's interpreted as referring to the
line above, and I do know your preference, but I stand by this particular
comment -- it follows the rule of explaining WHY we do something rather than
WHAT, and I believe in this case the why is very much not obvious* and worry
someone reading this code in the future might think this branch would actually
indicate a bug and decide to throw an exception or something. So I'll adjust
the spacing to clarify which line it's referring to but I would like to keep
it.
> I believe in this case the why is very much not obvious
\* Source: I myself did not remember about dynamic topics and only realized
this was possible when looking at some TTD tests
--
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
