kkonstantine commented on a change in pull request #10158:
URL: https://github.com/apache/kafka/pull/10158#discussion_r579395276
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/util/KafkaBasedLog.java
##########
@@ -161,6 +164,7 @@ public void start() {
// Create the topic admin client and initialize the topic ...
admin = topicAdminSupplier.get(); // may be null
+ useAdminForListOffsets = admin != null;
Review comment:
My main observation here is that keeping initialization in the
constructor allows us to declare read only fields as `final`.
It also offers better exception safety, in the sense that when an object is
constructed (the call to its constructor succeeds) then we know that more or
less we have a functional instance of that class.
I'm not ignoring the cases where the initialization of variables has to
happen at a later time during the call of `start`. But hopefully these cases
are minimal, or else `start` ends up being the actual constructor of objects
like this one here. Which can be redundant and in some cases more risky.
As I mentioned, since `admin` is not immutable here (can be set to `null`
later to support connection with older brokers) I'm fine keeping this call in
`start`. But I'd still recommend not pushing initializations outside the
constructor if they can happen at the time of the object construction (as is
the case with `admin`'s initial initialization here).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
