vamossagar12 commented on PR #12321: URL: https://github.com/apache/kafka/pull/12321#issuecomment-1161701481
> I think there's a better way to go about this. The general issue we're trying to address is that accessing null references can throw NPEs; there's nothing very special about the case of method references and `Utils::closeQuietly`. > > If we were to follow the logic in this PR as-is, we'd have to add `if (thing != null)` checks around every method invocation on `thing`, which would have to happen [here](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L259), [here](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L331), [here](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L467), [here](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L566), etc. > > A less-verbose approach that'd be just as safe is to rely on guarantees we can get at construction time for these objects: > > * The `configLog` field of `KafkaConfigBackingStore` is [declared final](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L256) and [initialized in the constructor](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java#L316); we can easily verify that it will never be null > * The `offsetStore` field of `AbstractWorkerSourceTask` is also [declared final](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L178), and accepted as a [constructor parameter](https://github.com/apache/kafka/blob/09286669877abaf17a1d9d52fa34c294e050f835/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractWorkerSourceTask.java#L214). Since we never pass in `null` as the `offsetStore` parameter to the constructor for this class, we can add a call to `Objects::requireNonNull` in the constructor that won't break any existing code, and will obviate the need for null checks in other parts of the class > > The reason we have a special case for this in `WorkerConnector` is that its `offsetStore` can be null, since that class is used for both source connectors (which an offset store is brought up for) and sink connectors (which no offset store is brought up for). Thanks for the detailed explanation Chris! I think this suggestion of yours is a lot neater. Also, I realised that on similar lines, I don't need a null check for `task` object in AbstractWorkerSourceTask as upstream it seems to be ensured that it would be setup properly (or throw an exception if the taskClass isn't proper). Let me know if this makes sense. -- 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
