jiafu1115 commented on code in PR #20203:
URL: https://github.com/apache/kafka/pull/20203#discussion_r2461480480
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -591,6 +600,15 @@ class BrokerServer(
"all of the SocketServer Acceptors to be started",
enableRequestProcessingFuture, startupDeadline, time)
+ brokerReadyCallbacks.foreach { callback =>
Review Comment:
@chia7712 Thanks a lot for the thoughtful review!
I completely understand your concern about potential over-engineering.
That said, I’d prefer to keep the collection for the following reasons:
1. BrokerReadyCallback is a public extension point — it’s defined in
server-common as a callback interface, which suggests it’s intended to
support
multiple implementations.
2. Low maintenance overhead — the collection adds only about 10 lines of
code,
with minimal impact on readability or maintenance.
3. Future-proofing — while RemoteLogManager is currently the only user, the
pattern
(registering callbacks during initialization and invoking them when ready)
is common
and flexible. If we ever need to support additional implementations (e.g.,
for other
modules), we won’t need to refactor this part of the code.
If we change the code as this style, it would look too customized with
flexible lost.
That said, if you feel strongly about simplifying this part, I’m happy to
revisit it. What are your thoughts?
BTW, I’ve adopted all the other suggestions. Thanks again for your review!
--
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]
