hachikuji commented on code in PR #12736: URL: https://github.com/apache/kafka/pull/12736#discussion_r993756766
########## metadata/src/main/java/org/apache/kafka/controller/BrokerHeartbeatManager.java: ########## @@ -414,7 +420,9 @@ void updateControlledShutdownOffset(int brokerId, long controlledShutDownOffset) throw new RuntimeException("Fenced brokers cannot enter controlled shutdown."); } active.remove(broker); - broker.controlledShutDownOffset = controlledShutDownOffset; + if (broker.controlledShutDownOffset < 0) { + broker.controlledShutDownOffset = controlledShutDownOffset; + } log.debug("Updated the controlled shutdown offset for broker {} to {}.", Review Comment: This log message is misleading if we don't update the offset. ########## metadata/src/main/java/org/apache/kafka/controller/BrokerHeartbeatManager.java: ########## @@ -275,6 +275,12 @@ Collection<BrokerHeartbeatState> brokers() { return brokers.values(); } + // VisibleForTesting + long getControlledShutDownOffset(int brokerId) { + return brokers.get(brokerId).controlledShutDownOffset; + } + + Review Comment: In the javadoc for this class, we mention that controlled shutdown is "soft state." This is no longer strictly true following KIP-841. Perhaps we could update the doc? ########## metadata/src/main/java/org/apache/kafka/controller/BrokerHeartbeatManager.java: ########## @@ -275,6 +275,12 @@ Collection<BrokerHeartbeatState> brokers() { return brokers.values(); } + // VisibleForTesting + long getControlledShutDownOffset(int brokerId) { Review Comment: nit: usually we drop the `get` prefix, so just `controlledShutdownOffset`. -- 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