mjsax commented on code in PR #19913:
URL: https://github.com/apache/kafka/pull/19913#discussion_r2146256034
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/InternalTopicManager.java:
##########
@@ -90,6 +92,8 @@ public InternalTopicManager(final Time time,
final StreamsConfig streamsConfig) {
this.time = time;
this.adminClient = adminClient;
+ this.isManualInternalTopicConfig =
streamsConfig.getString(StreamsConfig.INTERNAL_TOPIC_SETUP_CONFIG)
+
.equals(StreamsConfig.INTERNAL_TOPIC_SETUP_MANUAL);
Review Comment:
I would prefer to "flip" the check:
```
this.isManualInternalTopicConfig =
StreamsConfig.INTERNAL_TOPIC_SETUP_MANUAL.equals(
streamsConfig.getString(StreamsConfig.INTERNAL_TOPIC_SETUP_CONFIG)
);
```
The advantage is, that we know that `INTERNAL_TOPIC_SETUP_MANUAL` is never
`null`, so there no risk of a NPE. On the other hand, if
`streamsConfig.getString(StreamsConfig.INTERNAL_TOPIC_SETUP_CONFIG)` would
return `null` (what we still guard against, but this guard is weaker) we cannot
call `equals()` on the `null` reference and crash.
As a rule of thumb: I alway prefer to use `<constant>.equals(<variable>)`
instead of `<variable>.equals(<constant>)` to guard against NPE.
--
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
