hachikuji commented on code in PR #12736:
URL: https://github.com/apache/kafka/pull/12736#discussion_r993931747
##########
metadata/src/main/java/org/apache/kafka/controller/BrokerHeartbeatManager.java:
##########
@@ -276,7 +277,7 @@ Collection<BrokerHeartbeatState> brokers() {
}
// VisibleForTesting
- long getControlledShutDownOffset(int brokerId) {
+ long controlledShutDownOffset(int brokerId) {
Review Comment:
nit: could we call this `controlledShutdownOffset`. I think it is a bit more
conventional to see "shutdown" as one word, as in
`updateControlledShutdownOffset` below. (Similarly, perhaps we can rename the
field.)
Also, how about returning `OptionalLong` from this method to make it a bit
safer? We can return `OptionalLong.empty()` if either the brokerId is not
present in `brokers` or if `controlledShutdownOffset` is -1.
##########
metadata/src/main/java/org/apache/kafka/controller/BrokerHeartbeatManager.java:
##########
@@ -414,9 +421,11 @@ void updateControlledShutdownOffset(int brokerId, long
controlledShutDownOffset)
throw new RuntimeException("Fenced brokers cannot enter controlled
shutdown.");
}
active.remove(broker);
- broker.controlledShutDownOffset = controlledShutDownOffset;
- log.debug("Updated the controlled shutdown offset for broker {} to
{}.",
- brokerId, controlledShutDownOffset);
+ if (broker.controlledShutDownOffset < 0) {
Review Comment:
Perhaps we should call this `maybeUpdateControlledShutdownOffset` since the
update may or may not happen. We should also update the javadoc to describe the
change in behavior.
--
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
