ZOOKEEPER-2623: [ADDENDUM] Forbid OpCode.check outside OpCode.multi

Individual `OpCode.check` will get `UnimplementedException`.
apache · Jan 9, 2024 · dc22ba8 · dc22ba8
1 parent 9e40464
commit dc22ba8
Show file tree

Hide file tree

Showing 9 changed files with 16 additions and 29 deletions.
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
@@ -50,7 +50,6 @@
 import org.apache.zookeeper.data.Id;
 import org.apache.zookeeper.data.Stat;
 import org.apache.zookeeper.proto.AddWatchRequest;
-import org.apache.zookeeper.proto.CheckVersionRequest;
 import org.apache.zookeeper.proto.CheckWatchesRequest;
 import org.apache.zookeeper.proto.Create2Response;
 import org.apache.zookeeper.proto.CreateResponse;
@@ -355,10 +354,8 @@ public void processRequest(Request request) {
             }
             case OpCode.check: {
                 lastOp = "CHEC";
-                CheckVersionRequest checkVersionRequest = request.readRequestRecord(CheckVersionRequest::new);
-                path = checkVersionRequest.getPath();
-                handleCheckVersionRequest(checkVersionRequest, cnxn, request.authInfo);
-                requestPathMetricsCollector.registerRequest(request.type, path);
+                rsp = new SetDataResponse(rc.stat);
+                err = Code.get(rc.err);
                 break;
             }
             case OpCode.exists: {
@@ -655,19 +652,6 @@ private Record handleGetDataRequest(Record request, ServerCnxn cnxn, List<Id> au
         return new GetDataResponse(b, stat);
     }
 
-    private void handleCheckVersionRequest(CheckVersionRequest request, ServerCnxn cnxn, List<Id> authInfo) throws KeeperException {
-        String path = request.getPath();
-        DataNode n = zks.getZKDatabase().getNode(path);
-        if (n == null) {
-            throw new KeeperException.NoNodeException();
-        }
-        zks.checkACL(cnxn, zks.getZKDatabase().aclForNode(n), ZooDefs.Perms.READ, authInfo, path, null);
-        int version = request.getVersion();
-        if (version != -1 && version != n.stat.getVersion()) {
-            throw new KeeperException.BadVersionException(path);
-        }
-    }
-
     private boolean closeSession(ServerCnxnFactory serverCnxnFactory, long sessionId) {
         if (serverCnxnFactory == null) {
             return false;

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -809,6 +809,10 @@ private void pRequestHelper(Request request) {
                 SetACLRequest setAclRequest = request.readRequestRecord(SetACLRequest::new);
                 pRequest2Txn(request.type, zks.getNextZxid(), request, setAclRequest);
                 break;
+            case OpCode.check:
+                CheckVersionRequest checkRequest = request.readRequestRecord(CheckVersionRequest::new);
+                pRequest2Txn(request.type, zks.getNextZxid(), request, checkRequest);
+                break;
             case OpCode.multi:
                 MultiOperationRecord multiRequest;
                 try {

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java
@@ -304,8 +304,8 @@ static boolean isValid(int type) {
         // make sure this is always synchronized with Zoodefs!!
         switch (type) {
         case OpCode.notification:
-            return false;
         case OpCode.check:
+            return false;
         case OpCode.closeSession:
         case OpCode.create:
         case OpCode.create2:
@@ -361,6 +361,7 @@ public boolean isQuorum() {
         case OpCode.deleteContainer:
         case OpCode.setACL:
         case OpCode.setData:
+        case OpCode.check:
         case OpCode.multi:
         case OpCode.reconfig:
             return true;

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/CommitProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/CommitProcessor.java
@@ -181,6 +181,7 @@ protected boolean needCommit(Request request) {
         case OpCode.reconfig:
         case OpCode.multi:
         case OpCode.setACL:
+        case OpCode.check:
             return true;
         case OpCode.sync:
             return matchSyncs;

diff --git a/...per-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java b/...per-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
@@ -108,6 +108,7 @@ public void run() {
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     zks.getFollower().request(request);
                     break;
                 case OpCode.createSession:

diff --git a/...per-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java b/...per-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java
@@ -109,6 +109,7 @@ public void run() {
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     zks.getObserver().request(request);
                     break;
                 case OpCode.createSession:

diff --git a/...per-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java b/...per-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java
@@ -85,6 +85,7 @@ public void run() {
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     sendErrorResponse(request);
                     continue;
                 case OpCode.closeSession:

diff --git a/...er-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java b/...er-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java
@@ -147,6 +147,7 @@ static boolean isWriteOp(int requestType) {
         case ZooDefs.OpCode.reconfig:
         case ZooDefs.OpCode.setACL:
         case ZooDefs.OpCode.multi:
+        case ZooDefs.OpCode.check:
             return true;
         }
         return false;

diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/CheckTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/CheckTest.java
@@ -77,14 +77,7 @@ private static void checkVersion(TestableZooKeeper zk, String path, int version)
     private void testOperations(TestableZooKeeper zk) throws Exception {
         Stat stat = new Stat();
         zk.getData("/", false, stat);
-        checkVersion(zk, "/", -1);
-        checkVersion(zk, "/", stat.getVersion());
-        assertThrows(KeeperException.BadVersionException.class, () -> {
-            checkVersion(zk, "/", stat.getVersion() + 1);
-        });
-        assertThrows(KeeperException.NoNodeException.class, () -> {
-            checkVersion(zk, "/no-node", Integer.MAX_VALUE);
-        });
+        assertThrows(KeeperException.UnimplementedException.class, () -> checkVersion(zk, "/", -1));
     }
 
     @Test
@@ -96,7 +89,7 @@ public void testStandalone() throws Exception {
     public void testStandaloneDatabaseReloadAfterCheck() throws Exception {
         try {
             testOperations(createClient());
-        } catch (Exception ignored) {
+        } catch (Throwable ignored) {
             // Ignore to test database reload after check
         }
         stopServer();
@@ -131,7 +124,7 @@ public void testClusterDatabaseReloadAfterCheck() throws Exception {
 
             try {
                 testOperations(qb.createClient(new CountdownWatcher(), QuorumPeer.ServerState.LEADING));
-            } catch (Exception ignored) {
+            } catch (Throwable ignored) {
                 // Ignore to test database reload after check
             }
             qb.shutdown(leader);