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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
