[GitHub] [zookeeper] jonmv commented on a diff in pull request #1925: ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers

[via GitHub]

jonmv commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r1153570706


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java:
##
@@ -174,6 +224,21 @@ public void run() {
 break;
 }
 
+if (si == turnForwardingDelayOn) {
+nextProcessor.close();
+continue;
+}
+if (si == turnForwardingDelayOff) {
+nextProcessor.open();

Review Comment:
   `open()` opens the gate. What about `startDelaying()` and 
`flushAndStopDelaying()`? 



-- 
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

[GitHub] [zookeeper] jonmv commented on a diff in pull request #1925: ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers

[via GitHub]

jonmv commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r1153570079


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java:
##
@@ -174,6 +224,21 @@ public void run() {
 break;
 }
 
+if (si == turnForwardingDelayOn) {
+nextProcessor.close();

Review Comment:
   Only followers enqueue these special requests, so that can't happen. 
Observers don't ack txns, as far as I remember? 
   



-- 
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

[GitHub] [zookeeper] jonmv commented on a diff in pull request #1925: ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers

[via GitHub]

jonmv commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r1153562283


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java:
##
@@ -106,6 +106,8 @@ protected void setupRequestProcessors() {
 if (syncRequestProcessorEnabled) {
 syncProcessor = new SyncRequestProcessor(this, null);
 syncProcessor.start();
+} else {
+syncProcessor = null;

Review Comment:
   No, I'm just used to always assigning (to final fields). This can be removed 
again. 



-- 
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

[GitHub] [zookeeper] fanyang89 commented on a diff in pull request #1925: ZOOKEEPER-4541 Ephemeral znode owned by closed session visible in 1 of 3 servers

[via GitHub]

fanyang89 commented on code in PR #1925:
URL: https://github.com/apache/zookeeper/pull/1925#discussion_r1152851048


##
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java:
##
@@ -174,6 +224,21 @@ public void run() {
 break;
 }
 
+if (si == turnForwardingDelayOn) {
+nextProcessor.close();

Review Comment:
   At the ctor of SyncRequestProcessor, nextProcessor may be null. 
   Can this be an NPE at ObserverZooKeeperServer(with 
syncRequestProcessorEnabled=true)?



##
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverZooKeeperServer.java:
##
@@ -106,6 +106,8 @@ protected void setupRequestProcessors() {
 if (syncRequestProcessorEnabled) {
 syncProcessor = new SyncRequestProcessor(this, null);
 syncProcessor.start();
+} else {
+syncProcessor = null;

Review Comment:
   syncProcessor as an ObserverZooKeeperServer field should have a default 
value of null.
   Does setting null here makes a difference?



##
zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java:
##
@@ -174,6 +224,21 @@ public void run() {
 break;
 }
 
+if (si == turnForwardingDelayOn) {
+nextProcessor.close();
+continue;
+}
+if (si == turnForwardingDelayOff) {
+nextProcessor.open();

Review Comment:
   This naming here is confusing. 
   The intention here is: to get the turning delaying off request, open the 
gate, then flush all pending requests to the downstream processor.
   nextProcessor.open() is to open the gate or turn the delay on? 



-- 
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