kezhuw commented on code in PR #2154:
URL: https://github.com/apache/zookeeper/pull/2154#discussion_r1766881738
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerZooKeeperServer.java:
##########
@@ -152,24 +152,25 @@ protected void unregisterJMX(Learner peer) {
}
@Override
- public synchronized void shutdown() {
+ public synchronized void shutdown(boolean fullyShutDown) {
if (!canShutdown()) {
+ super.shutdown(fullyShutDown);
LOG.debug("ZooKeeper server is not running, so not proceeding to
shutdown!");
return;
}
Review Comment:
This does not look nice. This overriding hierarchy tends to log repeated,
both "Shutting down" and above "not proceeding to shutdown".
Can we introduce another method (say, shutdownDirectly/shutdownComponenets
or whatever suitable) to cover remaining shutdown logic for override, so this
`canShutdown` branch does not need to be repeated in overriding method ?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SendAckRequestProcessor.java:
##########
@@ -46,7 +48,7 @@ public void processRequest(Request si) {
learner.writePacket(qp, false);
} catch (IOException e) {
LOG.warn("Closing connection to leader, exception during
packet send", e);
- learner.closeSockSync();
+ learner.closeSocket();
Review Comment:
+1 on this.
This is the leftover of ZOOKEEPER-4409. We should respect
`zookeeper.learner.closeSocketAsync` 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.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
