zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From shralex <...@git.apache.org>
Subject [GitHub] zookeeper pull request #411: ZOOKEEPER-2684 Fix a crashing bug in the mixed ...
Date Mon, 30 Oct 2017 22:39:42 GMT
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 ?


---

Mime
View raw message