ZOOKEEPER-3594: Ability to skip proposing requests with error transactions #1172

vladimirivic · 2019-12-10T22:50:22Z

Ensembles that have a high write request rate could be skipping proposing requests that contain errors instead of having the quorum vote on those transactions.

For example, having a sizable write traffic that results in error transactions would be creating additional network and log disk space overhead for each of the requests that would only return errors to the client in the end (e.g. delete non-existing path, version mismatch, trying to create a node that already exists etc).

Currently, there is no such logic in the ProposalRequestProcessor, every request that comes out of PrepRequestProcessor will be proposed to the quorum.

Proposed solution workflow:

A client sends a new write request trying to setData on an non-existing node
The server accepts the request and sends it through the PrepRequestProcessor
PrepRequestProcessor detects the error and assigns the error transaction to the request
Between PrepRequestProcessor and ProposalRequestProcessor there is another processor named SkipRequestProcessor which sole responsibility is to decide will the request be forwarded or returned to the originating quorum peer (Leader or Learner).
The quorum peer waits for all previous requests to complete before the error request proceeds with echoing the error back to the client.
Requirements:

We should be conservative about the use of ZXID. If the request generates an error transaction we don't want to increment the last proposed ZXID and cause any gaps in the log.
Request that are found to be invalid should be sent directly to the origin (either to the corresponding Learner or to the Leader directly) so there is exactly one roundtrip for each request with error transaction.
All the requests must preserve its order, the changes must be backwards compatible with the Zookeeper ordering guarantees.
Challenges:

Skipping request without having them go through the proposal pipeline poses a challenge in preserving Zookeeper transaction order.
Avoiding any additional changes inside CommitProcessor if possible.
Have unified logic for all three different paths in which write requests can come through:
Via Leader placed directly into the PrepRequestProcessor
Via Follower where the request is forwarded to the leader and also passed to CommitProcessor to wait for COMMIT packet
Via Observer, where the request is forwarded to the Leader and the Observer waits for the INFORM packet

https://issues.apache.org/jira/browse/ZOOKEEPER-3594

…tions

asf-ci · 2019-12-10T22:59:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1704/

Build result: FAILURE

[...truncated 498.45 KB...][JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/pom.xml to org.apache.zookeeper/parent/3.6.0-SNAPSHOT/parent-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-lock/pom.xml to org.apache.zookeeper/zookeeper-recipes-lock/3.6.0-SNAPSHOT/zookeeper-recipes-lock-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/pom.xml to org.apache.zookeeper/zookeeper-contrib/3.6.0-SNAPSHOT/zookeeper-contrib-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/pom.xml to org.apache.zookeeper/zookeeper-client/3.6.0-SNAPSHOT/zookeeper-client-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/pom.xml to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-jute/target/zookeeper-jute-3.6.0-SNAPSHOT-javadoc.jar to org.apache.zookeeper/zookeeper-jute/3.6.0-SNAPSHOT/zookeeper-jute-3.6.0-SNAPSHOT-javadoc.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/zookeeper-recipes-queue/pom.xml to org.apache.zookeeper/zookeeper-recipes-queue/3.6.0-SNAPSHOT/zookeeper-recipes-queue-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/pom.xml to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT-tests.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT-tests.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-docs/target/zookeeper-docs-3.6.0-SNAPSHOT-sources.jar to org.apache.zookeeper/zookeeper-docs/3.6.0-SNAPSHOT/zookeeper-docs-3.6.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-recipes/pom.xml to org.apache.zookeeper/zookeeper-recipes/3.6.0-SNAPSHOT/zookeeper-recipes-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/pom.xml to org.apache.zookeeper/zookeeper-client-c/3.6.0-SNAPSHOT/zookeeper-client-c-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-rest/pom.xml to org.apache.zookeeper/zookeeper-contrib-rest/3.6.0-SNAPSHOT/zookeeper-contrib-rest-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-assembly/pom.xml to org.apache.zookeeper/zookeeper-assembly/3.6.0-SNAPSHOT/zookeeper-assembly-3.6.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-contrib/zookeeper-contrib-zooinspector/pom.xml to org.apache.zookeeper/zookeeper-contrib-zooinspector/3.6.0-SNAPSHOT/zookeeper-contrib-zooinspector-3.6.0-SNAPSHOT.pomchannel stopped[SpotBugs] Skipping execution of recorder since overall result is 'FAILURE'Setting status of 6616250 to FAILURE with url https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1704/ and message: 'FAILURE 'Using context: JenkinsMaven

hanm

some quick comments on errors I find when trying to rebuild this branch locally...

hanm · 2019-12-13T00:22:03Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerZooKeeperServer.java

 import org.apache.zookeeper.server.RequestProcessor;
 import org.apache.zookeeper.server.ServerMetrics;
 import org.apache.zookeeper.server.SyncRequestProcessor;
+import org.apache.zookeeper.server.TxnLogEntry;


I don't see where this is defined. Maybe in a new file that's forgot to be included as part of this PR?

This one should not be here.

hanm · 2019-12-13T00:23:37Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java

            peerLastZxid = ss.getLastZxid();

+
+            ableToSkipTxn = QuorumZooKeeperServer.isSkipTxnAvailable(learnerProtocolVersion);


ableToSkipTxn and learnerProtocolVersion are not declared.

Same for this one, I will update this part.

hanm · 2019-12-13T00:56:52Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/SkippedRequestQueue.java

+            if (lastZxid > request.zxid) {
+                ServerMetrics.getMetrics().QUORUM_PEER_SKIP_OUT_OF_ORDER.add(1);
+                LOG.error("Skip request is out of order!!!");
+                System.exit(ExitCode.QUORUM_PEER_SKIP_OUT_OF_ORDER.getValue());


Please use ServiceUtils.requestSystemExit instead. For reference: #1147

hanm

add more review comments.

hanm · 2019-12-20T19:04:35Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java

    }

+    public static boolean isSkipTxnAvailable(int learnerProtocolVersion) {
+        return true;


Are we sure that we always want to return true here as opposed to have a system property control the return value? Also, learnerProtocolVersion is not used.. is this intentional?

It would be also good to add a brief comment on what this does - it's not very obvious to me in first glance.

It is intentional when the learner protocol version is used but I wont be adding that change to reduce the footprint of this pull request. I will refactor this in a followup commit. I will use the system variable.

hanm · 2019-12-20T22:01:10Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverMaster.java

        sendPacket(pkt);
    }

+    synchronized void proposalSkipped(QuorumPacket qp) {


Is it possible to reuse the existing sendPacket ? The only difference is sendPacket also updates lastProposedZxid with the packet zxid.

That may be possible. I will explore that option. One problem I see is that a skip request can override lastProposedZxid for a valid request that is in flight.

hanm · 2019-12-21T00:17:26Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java


    long getNextZxid() {
-        return hzxid.incrementAndGet();
+        return hzxid.get() + 1;


this will not be thread safe, right? would this be a concern?

Won't be a concern as long as we keep processing requests in order inside PrepRequestProcessor. If that changes this can break but that is not likely to happen.

hanm · 2019-12-21T00:23:41Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

+        return hzxid.get() + 1;
+    }
+
+    void incrementZxid() {


another idea - instead of having this to increase the zxid we can instead have a decrementZxid, which will only be called if we have an error transaction. By doing that, we can keep the existing implementation of getNextZxid . This way we only decrement zxid for error transactions; for usual transactions we only call getNextZxid once (as opposed to the call both getNextZxid and incrementZxid ).

That is a great alternative and it was my first approach but then I though we need to remember the decrement zxid which I wanted to eliminate. But that was early when I started working on this. I will take another look and see how things look now if I use decrement zxid.

hanm · 2019-12-21T02:28:17Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java

+     * The request will be immediately sent back to the learner if there is no pending commits
+     * otherwise the request will be put inside the handler's queue to wait for pending commit
+     * to complete before is sent so we make sure we send all requests in order.
+     */


we can remove these comments, they are duplicate of the same comments on the SkipRequestHandler interface.

hanm · 2019-12-21T05:44:59Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java

+        return owner == ServerCnxn.me;
+    }
+
+    public boolean isFromFollower() {


this method is not used.. is this needed?

hanm · 2019-12-21T05:49:18Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java

+     *
+     */
+    public boolean canSkip() {
+        return skipRequestHandler != null;


Wouldn't this always return true? As far as I can see skipRequestHandler will never be null as it's set by setSkipRequestHandler

True, this is related to learner protocol version, something outside the scope of this. This is not required to be in this pull request.

hanm · 2019-12-21T05:52:16Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderHandler.java

+     * The request will be immediately sent back to the learner if there is no pending commits
+     * otherwise the request will be put inside the handler's queue to wait for pending commit
+     * to complete before is sent so we make sure we send all requests in order.
+     */


comments can be removed (same comments already present in interface).

hanm · 2019-12-21T05:52:34Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderHandler.java

+     * LeaderHandler commits directly in it's own commit processor.
+     * LearnerHandler must send SKIP message to the Learners so they can
+     * commit the request in their own commit processor.
+     */


same - duplicate comments with interface that can be removed.

hanm · 2019-12-21T05:52:49Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LeaderHandler.java

+ *   that occurred right before that commit.
+ */
+public class LeaderHandler implements SkipRequestHandler {
+    private static final Logger LOG = LoggerFactory.getLogger(LeaderHandler.class);


logger is never used.. remove?

vladimirivic

@hanm thanks for the through review! First time porting changes to open source. I will update the pull request with the changes after the holidays.

ZOOKEEPER-3594: Ability to skip proposing requests with error transac…

6616250

…tions

hanm requested changes Dec 13, 2019

View reviewed changes

hanm reviewed Dec 13, 2019

View reviewed changes

hanm requested changes Dec 21, 2019

View reviewed changes

vladimirivic commented Dec 22, 2019

View reviewed changes

kezhuw mentioned this pull request Sep 27, 2023

ZOOKEEPER-3594: Don't propose error transactions #2070

Open

ztzg force-pushed the master branch from 1c60545 to e2070be Compare October 3, 2023 12:57

		peerLastZxid = ss.getLastZxid();


		ableToSkipTxn = QuorumZooKeeperServer.isSkipTxnAvailable(learnerProtocolVersion);

ZOOKEEPER-3594: Ability to skip proposing requests with error transactions #1172

Are you sure you want to change the base?

ZOOKEEPER-3594: Ability to skip proposing requests with error transactions #1172

Uh oh!

Conversation

vladimirivic commented Dec 10, 2019

Uh oh!

asf-ci commented Dec 10, 2019

Build result: FAILURE

Uh oh!

hanm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimirivic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants