You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by kfirlevari <gi...@git.apache.org> on 2017/10/30 10:42:41 UTC
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
GitHub user kfirlevari opened a pull request:
https://github.com/apache/zookeeper/pull/411
ZOOKEEPER-2684 Fix a crashing bug in the mixed workloads commit processor
We wish to fix this long-standing issue in the code.
Note that the previous commit processor algorithm had the same approach (as the one suggested in this fix) when dealing with a request that has a different cxid than session's expected one (see [here](https://github.com/apache/zookeeper/commit/9fc632c4f0a340b0a00ec6dff39c7b454c802822#diff-5cc688a027068714af01b0ad4d292fe5L238)).
This fix is based on the code from https://github.com/apache/zookeeper/pull/167, following the discussion in https://issues.apache.org/jira/browse/ZOOKEEPER-2684 .
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kfirlevari/zookeeper ZOOKEEPER-2684
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/411.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #411
----
commit 7e67982430d1f88fc50a6e60aceb168851a4c88c
Author: Kfir Lev-Ari <kl...@apple.com>
Date: 2017-10-30T10:29:58Z
ZOOKEEPER-2684 Fix a crashing bug in the mixed workloads commit processor
----
---
[GitHub] zookeeper issue #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed workloa...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on the issue:
https://github.com/apache/zookeeper/pull/411
The build failed because _testNodeDataChanged_ failed (a test that doesn't seem related to the current change).
It seems like zk2.create at line 122 ([WatchEventWhenAutoResetTest.java](https://github.com/apache/zookeeper/blob/master/src/java/test/org/apache/zookeeper/test/WatchEventWhenAutoResetTest.java#L122)) is taking too long, and therefore we receive the NodeDeleted event instead of the expected NodeDataChanged (locally on my machine it didn't fail). I guess adding some kind of sleep prior to line 124 should solve this issue?
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by shralex <gi...@git.apache.org>.
Github user shralex commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r147854385
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java ---
@@ -376,4 +377,123 @@ public void noStarvationOfReadRequestsTest() throws Exception {
!processedRequests.contains(r));
}
}
+
+ /**
+ * In the following test, we verify that we can handle the case that we got a commit
+ * of a request we never seen since the session that we just established. This can happen
+ * when a session is just established and there is request waiting to be committed in the
+ * in the session queue but it sees a commit for a request that belongs to the previous connection.
+ */
+ @Test(timeout = 1000)
+ public void noCrashOnCommittedRequestsOfUnseenRequestTest() throws Exception {
+ final String path = "/noCrash/OnCommittedRequests/OfUnseenRequestTest";
+ final int numberofReads = 10;
+ final int sessionid = 0x123456;
+ final int firstCXid = 0x100;
+ int readReqId = firstCXid;
+ processor.stoppedMainLoop = true;
+ HashSet<Request> localRequests = new HashSet<Request>();
+ // queue the blocking write request to queuedRequests
+ Request firstCommittedReq = newRequest(
+ new CreateRequest(path, new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT_SEQUENTIAL.toFlag()),
+ OpCode.create, sessionid, readReqId++);
+ processor.queuedRequests.add(firstCommittedReq);
+ localRequests.add(firstCommittedReq);
+
+ // queue read requests to queuedRequests
+ for (; readReqId <= numberofReads+firstCXid; ++readReqId) {
+ Request readReq = newRequest(new GetDataRequest(path, false),
+ OpCode.getData, sessionid, readReqId);
+ processor.queuedRequests.add(readReq);
+ localRequests.add(readReq);
+ }
+
+ //run once
+ Assert.assertTrue(processor.queuedRequests.containsAll(localRequests));
+ processor.initThreads(numberofReads* 2);
+ processor.run();
+
+ //We verify that the processor is waiting for the commit
+ Assert.assertTrue(processedRequests.isEmpty());
+
+ // We add a commit that belongs to the same session but with smaller cxid,
+ // i.e., commit of an update from previous connection of this session.
+ Request preSessionCommittedReq = newRequest(
+ new CreateRequest(path, new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT_SEQUENTIAL.toFlag()),
+ OpCode.create, sessionid, firstCXid - 2);
+ processor.committedRequests.add(preSessionCommittedReq);
+ processor.committedRequests.add(firstCommittedReq);
+ processor.run();
+
+ //We verify that the commit processor processed the old commit prior to the newer messages
+ Assert.assertTrue(processedRequests.peek() == preSessionCommittedReq);
+
+ processor.run();
+
+ //We verify that the commit processor handle all messages.
+ Assert.assertTrue(processedRequests.containsAll(localRequests));
+ }
+
+ /**
+ * In the following test, we verify if we handle the case in which we get a commit
+ * for a request that has higher Cxid than the one we are waiting. This can happen
+ * when a session connection is lost but there is a request waiting to be committed in the
+ * session queue. However, since the session has moved, new requests can get to
+ * the leader out of order. Hence, the commits can also arrive "out of order" w.r.t. cxid.
+ * We should commit the requests according to the order we receive from the leader, i.e., wait for the relevant commit.
--- End diff --
1) I think its worth while explaining the two scenarios in the CommitProcessor.java comments. I mean how its possible to get higher cxid and lower cxid from leader compared to what you expect. Including the part where requests may reach the leader out of cxid order if session is moved.
2) I wonder if in this case (receiving higher cxid from leader) we should also empty the local pending requests ? What's the value of keeping the requests in the local queue ?
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/411
---
[GitHub] zookeeper issue #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed workloa...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/411
@kfirlevari yeah, that's an outstanding flaky test.
Here's the jira about it: https://issues.apache.org/jira/browse/ZOOKEEPER-2807
Amend your latest commit to trigger another Jenkins build.
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148119207
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -246,33 +246,51 @@ public void run() {
}
/*
- * Check if request is pending, if so, update it with the
- * committed info
+ * Check if request is pending, if so, update it with the committed info
*/
LinkedList<Request> sessionQueue = pendingRequests
.get(request.sessionId);
if (sessionQueue != null) {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // TL;DR - we should not encounter this scenario often under normal load.
--- End diff --
NP ;)
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148014638
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -255,24 +255,23 @@ public void run() {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // we can get commit requests that are not at the queue head after
+ // a session moved (see ZOOKEEPER-2684). We will just pass the
+ // commit to the next processor and put the pending back with
+ // a warning, we should not see this often under normal load
+ LOG.warn("Got request " + request +
+ " but we are expecting request " + topPending);
+ sessionQueue.addFirst(topPending);
+ } else {
+ /*
+ * We want to send our version of the request. the
+ * pointer to the connection in the request
+ */
+ topPending.setHdr(request.getHdr());
--- End diff --
I added an explanation, let me know if you meant something more specific.
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r147854977
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java ---
@@ -376,4 +377,123 @@ public void noStarvationOfReadRequestsTest() throws Exception {
!processedRequests.contains(r));
}
}
+
+ /**
+ * In the following test, we verify that we can handle the case that we got a commit
+ * of a request we never seen since the session that we just established. This can happen
+ * when a session is just established and there is request waiting to be committed in the
+ * in the session queue but it sees a commit for a request that belongs to the previous connection.
--- End diff --
typo: "in the in the"
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r147883484
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -255,24 +255,23 @@ public void run() {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // we can get commit requests that are not at the queue head after
+ // a session moved (see ZOOKEEPER-2684). We will just pass the
+ // commit to the next processor and put the pending back with
+ // a warning, we should not see this often under normal load
+ LOG.warn("Got request " + request +
+ " but we are expecting request " + topPending);
+ sessionQueue.addFirst(topPending);
+ } else {
+ /*
+ * We want to send our version of the request. the
+ * pointer to the connection in the request
+ */
+ topPending.setHdr(request.getHdr());
--- End diff --
Would you mind explaining why we normally want to send our version of the request and why it is ok not to in this case?
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148115810
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -246,33 +246,51 @@ public void run() {
}
/*
- * Check if request is pending, if so, update it with the
- * committed info
+ * Check if request is pending, if so, update it with the committed info
*/
LinkedList<Request> sessionQueue = pendingRequests
.get(request.sessionId);
if (sessionQueue != null) {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // TL;DR - we should not encounter this scenario often under normal load.
+ // We pass the commit to the next processor and put the pending back with a warning.
+
+ // Generally, we can get commit requests that are not at the queue head after
+ // a session moved (see ZOOKEEPER-2684). Let's denote the previous server of the session
+ // with A, and the server that the session moved to with B (keep in mind that it is
+ // possible that the session already moved from B to a new server C, and maybe C=A).
+ // 1. If request.cxid < topPending.cxid : this means that the session requested this update
+ // from A, then moved to B (i.e., which is us), and now B receives the commit
+ // for the update after the session already performed several operations in B
+ // (and therefore its cxid is higher than that old request).
+ // 2. If request.cxid > topPending.cxid : this means that the session requested an updated
+ // from B with cxid that is bigger than the one we know therefore in this case we
+ // are A, and we lost the connection to the session. Given that we are waiting for a commit
+ // for that update, it means that we already sent the request to the leader and it will
+ // be committed at some point (in this case the order of cxid won't follow zxid, since zxid
+ // is an increasing order). It is not safe for us to delete the session's queue at this
+ // point, since it is possible that the session has newer requests in it after it moved
+ // back to us. We just leave the queue as it is, and once the commit arrives (for the old
+ // request), the finalRequestProcessor will see a closed cnxn handle, and just won't send a
+ // response.
+ // Also note that we don't have a local session, therefore we treat the request
+ // like any other commit for a remote request, i.e., we perform the update without sending
+ // a response.
+
+ LOG.warn("Got request " + request +
+ " but we are expecting request " + topPending);
+ sessionQueue.addFirst(topPending);
+ } else {
+ // We want to send to the next processor our version of the request,
+ // since it contains the session information that is needed
+ // for post update processing (e.g., using request.cnxn we send a response to the client).
+ topPending.setHdr(request.getHdr());
--- End diff --
The explanation is really great. I was also hoping you could shed some light on this part of the code. Why do we "want to send to the next processor our version of the request" and why can we proceed in the other case if this is required here?
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148119340
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -246,33 +246,51 @@ public void run() {
}
/*
- * Check if request is pending, if so, update it with the
- * committed info
+ * Check if request is pending, if so, update it with the committed info
*/
LinkedList<Request> sessionQueue = pendingRequests
.get(request.sessionId);
if (sessionQueue != null) {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // TL;DR - we should not encounter this scenario often under normal load.
+ // We pass the commit to the next processor and put the pending back with a warning.
+
+ // Generally, we can get commit requests that are not at the queue head after
+ // a session moved (see ZOOKEEPER-2684). Let's denote the previous server of the session
+ // with A, and the server that the session moved to with B (keep in mind that it is
+ // possible that the session already moved from B to a new server C, and maybe C=A).
+ // 1. If request.cxid < topPending.cxid : this means that the session requested this update
+ // from A, then moved to B (i.e., which is us), and now B receives the commit
+ // for the update after the session already performed several operations in B
+ // (and therefore its cxid is higher than that old request).
+ // 2. If request.cxid > topPending.cxid : this means that the session requested an updated
+ // from B with cxid that is bigger than the one we know therefore in this case we
+ // are A, and we lost the connection to the session. Given that we are waiting for a commit
+ // for that update, it means that we already sent the request to the leader and it will
+ // be committed at some point (in this case the order of cxid won't follow zxid, since zxid
+ // is an increasing order). It is not safe for us to delete the session's queue at this
+ // point, since it is possible that the session has newer requests in it after it moved
+ // back to us. We just leave the queue as it is, and once the commit arrives (for the old
+ // request), the finalRequestProcessor will see a closed cnxn handle, and just won't send a
+ // response.
+ // Also note that we don't have a local session, therefore we treat the request
+ // like any other commit for a remote request, i.e., we perform the update without sending
+ // a response.
+
+ LOG.warn("Got request " + request +
+ " but we are expecting request " + topPending);
+ sessionQueue.addFirst(topPending);
+ } else {
+ // We want to send to the next processor our version of the request,
+ // since it contains the session information that is needed
+ // for post update processing (e.g., using request.cnxn we send a response to the client).
+ topPending.setHdr(request.getHdr());
--- End diff --
Updated accordingly
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148113899
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -246,33 +246,51 @@ public void run() {
}
/*
- * Check if request is pending, if so, update it with the
- * committed info
+ * Check if request is pending, if so, update it with the committed info
*/
LinkedList<Request> sessionQueue = pendingRequests
.get(request.sessionId);
if (sessionQueue != null) {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // TL;DR - we should not encounter this scenario often under normal load.
--- End diff --
I hate to be "that guy" but we generally use "/*" comments for long blocks like this.
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148014302
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java ---
@@ -376,4 +377,123 @@ public void noStarvationOfReadRequestsTest() throws Exception {
!processedRequests.contains(r));
}
}
+
+ /**
+ * In the following test, we verify that we can handle the case that we got a commit
+ * of a request we never seen since the session that we just established. This can happen
+ * when a session is just established and there is request waiting to be committed in the
+ * in the session queue but it sees a commit for a request that belongs to the previous connection.
+ */
+ @Test(timeout = 1000)
+ public void noCrashOnCommittedRequestsOfUnseenRequestTest() throws Exception {
+ final String path = "/noCrash/OnCommittedRequests/OfUnseenRequestTest";
+ final int numberofReads = 10;
+ final int sessionid = 0x123456;
+ final int firstCXid = 0x100;
+ int readReqId = firstCXid;
+ processor.stoppedMainLoop = true;
+ HashSet<Request> localRequests = new HashSet<Request>();
+ // queue the blocking write request to queuedRequests
+ Request firstCommittedReq = newRequest(
+ new CreateRequest(path, new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT_SEQUENTIAL.toFlag()),
+ OpCode.create, sessionid, readReqId++);
+ processor.queuedRequests.add(firstCommittedReq);
+ localRequests.add(firstCommittedReq);
+
+ // queue read requests to queuedRequests
+ for (; readReqId <= numberofReads+firstCXid; ++readReqId) {
+ Request readReq = newRequest(new GetDataRequest(path, false),
+ OpCode.getData, sessionid, readReqId);
+ processor.queuedRequests.add(readReq);
+ localRequests.add(readReq);
+ }
+
+ //run once
+ Assert.assertTrue(processor.queuedRequests.containsAll(localRequests));
+ processor.initThreads(numberofReads* 2);
+ processor.run();
+
+ //We verify that the processor is waiting for the commit
+ Assert.assertTrue(processedRequests.isEmpty());
+
+ // We add a commit that belongs to the same session but with smaller cxid,
+ // i.e., commit of an update from previous connection of this session.
+ Request preSessionCommittedReq = newRequest(
+ new CreateRequest(path, new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT_SEQUENTIAL.toFlag()),
+ OpCode.create, sessionid, firstCXid - 2);
+ processor.committedRequests.add(preSessionCommittedReq);
+ processor.committedRequests.add(firstCommittedReq);
+ processor.run();
+
+ //We verify that the commit processor processed the old commit prior to the newer messages
+ Assert.assertTrue(processedRequests.peek() == preSessionCommittedReq);
+
+ processor.run();
+
+ //We verify that the commit processor handle all messages.
+ Assert.assertTrue(processedRequests.containsAll(localRequests));
+ }
+
+ /**
+ * In the following test, we verify if we handle the case in which we get a commit
+ * for a request that has higher Cxid than the one we are waiting. This can happen
+ * when a session connection is lost but there is a request waiting to be committed in the
+ * session queue. However, since the session has moved, new requests can get to
+ * the leader out of order. Hence, the commits can also arrive "out of order" w.r.t. cxid.
+ * We should commit the requests according to the order we receive from the leader, i.e., wait for the relevant commit.
--- End diff --
Regarding 2 - I've answered the question also in the new comments. I think we shouldn't empty the queue because it is possible that the session returned to us (i.e., moved back). It does no harm to keep the old requests, given that the cnxn handle in the old request shows that the old connection is closed.
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by shralex <gi...@git.apache.org>.
Github user shralex commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148117464
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/CommitProcessor.java ---
@@ -246,33 +246,51 @@ public void run() {
}
/*
- * Check if request is pending, if so, update it with the
- * committed info
+ * Check if request is pending, if so, update it with the committed info
*/
LinkedList<Request> sessionQueue = pendingRequests
.get(request.sessionId);
if (sessionQueue != null) {
// If session queue != null, then it is also not empty.
Request topPending = sessionQueue.poll();
if (request.cxid != topPending.cxid) {
- LOG.error(
- "Got cxid 0x"
- + Long.toHexString(request.cxid)
- + " expected 0x" + Long.toHexString(
- topPending.cxid)
- + " for client session id "
- + Long.toHexString(request.sessionId));
- throw new IOException("Error: unexpected cxid for"
- + "client session");
+ // TL;DR - we should not encounter this scenario often under normal load.
+ // We pass the commit to the next processor and put the pending back with a warning.
+
+ // Generally, we can get commit requests that are not at the queue head after
+ // a session moved (see ZOOKEEPER-2684). Let's denote the previous server of the session
+ // with A, and the server that the session moved to with B (keep in mind that it is
+ // possible that the session already moved from B to a new server C, and maybe C=A).
+ // 1. If request.cxid < topPending.cxid : this means that the session requested this update
+ // from A, then moved to B (i.e., which is us), and now B receives the commit
+ // for the update after the session already performed several operations in B
+ // (and therefore its cxid is higher than that old request).
+ // 2. If request.cxid > topPending.cxid : this means that the session requested an updated
+ // from B with cxid that is bigger than the one we know therefore in this case we
+ // are A, and we lost the connection to the session. Given that we are waiting for a commit
+ // for that update, it means that we already sent the request to the leader and it will
+ // be committed at some point (in this case the order of cxid won't follow zxid, since zxid
+ // is an increasing order). It is not safe for us to delete the session's queue at this
+ // point, since it is possible that the session has newer requests in it after it moved
+ // back to us. We just leave the queue as it is, and once the commit arrives (for the old
+ // request), the finalRequestProcessor will see a closed cnxn handle, and just won't send a
+ // response.
+ // Also note that we don't have a local session, therefore we treat the request
+ // like any other commit for a remote request, i.e., we perform the update without sending
+ // a response.
+
+ LOG.warn("Got request " + request +
+ " but we are expecting request " + topPending);
+ sessionQueue.addFirst(topPending);
+ } else {
+ // We want to send to the next processor our version of the request,
+ // since it contains the session information that is needed
+ // for post update processing (e.g., using request.cnxn we send a response to the client).
+ topPending.setHdr(request.getHdr());
--- End diff --
My understanding is that when a request is in the local queue, there is (or could be) a client attached to this server waiting for a response, and there is other bookeeping of requests that are outstanding and have originated from this server (e.g., for setting the max outstanding requests) - we need to update this info when an outstanding request completes. In the other case, the operation originated from a different server and there is no local bookeeeping or a local client session that needs to be notified.
---
[GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Posted by kfirlevari <gi...@git.apache.org>.
Github user kfirlevari commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/411#discussion_r148014452
--- Diff: src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorConcurrencyTest.java ---
@@ -376,4 +377,123 @@ public void noStarvationOfReadRequestsTest() throws Exception {
!processedRequests.contains(r));
}
}
+
+ /**
+ * In the following test, we verify that we can handle the case that we got a commit
+ * of a request we never seen since the session that we just established. This can happen
+ * when a session is just established and there is request waiting to be committed in the
+ * in the session queue but it sees a commit for a request that belongs to the previous connection.
--- End diff --
Fixed
---