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

ZOOKEEPER-2623: [ADDENDUM] Forbid OpCode.check outside OpCode.multi
Individual `OpCode.check` will get `UnimplementedException`.
Reviewers: anmolnar, shoothzj, tisonkun
Author: kezhuw
Closes #2104 from kezhuw/ZOOKEEPER-2623-addendum-forbid-opcode-check-outside-multi

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

@@ -50,7 +50,6 @@ import org.apache.zookeeper.data.ACL;
 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 class FinalRequestProcessor implements RequestProcessor {
             }
             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 @@ public class FinalRequestProcessor implements RequestProcessor {
         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;

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

@@ -806,6 +806,10 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements Req
                 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 {

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

@@ -295,8 +295,8 @@ public class Request {
         // 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:
@@ -352,6 +352,7 @@ public class Request {
         case OpCode.deleteContainer:
         case OpCode.setACL:
         case OpCode.setData:
+        case OpCode.check:
         case OpCode.multi:
         case OpCode.reconfig:
             return true;

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

@@ -181,6 +181,7 @@ public class CommitProcessor extends ZooKeeperCriticalThread implements RequestP
         case OpCode.reconfig:
         case OpCode.multi:
         case OpCode.setACL:
+        case OpCode.check:
             return true;
         case OpCode.sync:
             return matchSyncs;

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

@@ -108,6 +108,7 @@ public class FollowerRequestProcessor extends ZooKeeperCriticalThread implements
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     zks.getFollower().request(request);
                     break;
                 case OpCode.createSession:

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

@@ -109,6 +109,7 @@ public class ObserverRequestProcessor extends ZooKeeperCriticalThread implements
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     zks.getObserver().request(request);
                     break;
                 case OpCode.createSession:

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

@@ -85,6 +85,7 @@ public class ReadOnlyRequestProcessor extends ZooKeeperCriticalThread implements
                 case OpCode.reconfig:
                 case OpCode.setACL:
                 case OpCode.multi:
+                case OpCode.check:
                     sendErrorResponse(request);
                     continue;
                 case OpCode.closeSession:

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java

@@ -147,6 +147,7 @@ public class RequestPathMetricsCollector {
         case ZooDefs.OpCode.reconfig:
         case ZooDefs.OpCode.setACL:
         case ZooDefs.OpCode.multi:
+        case ZooDefs.OpCode.check:
             return true;
         }
         return false;

zookeeper-server/src/test/java/org/apache/zookeeper/test/CheckTest.java

@@ -77,14 +77,7 @@ public class CheckTest extends ClientBase {
     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 class CheckTest extends ClientBase {
     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 class CheckTest extends ClientBase {
 
             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);