From 6d1725b2bdd8d91f04ef1faa771694f6dfbe1140 Mon Sep 17 00:00:00 2001
From: Kezhu Wang <kezhuw@gmail.com>
Date: Mon, 8 Jan 2024 19:07:47 +0800
Subject: [PATCH] ZOOKEEPER-2623: [ADDENDUM] Forbid OpCode.check outside
OpCode.multi
Individual `OpCode.check` will get `UnimplementedException`.
---
.../server/FinalRequestProcessor.java | 20 ++-----------------
.../server/PrepRequestProcessor.java | 4 ++++
.../org/apache/zookeeper/server/Request.java | 3 ++-
.../server/quorum/CommitProcessor.java | 1 +
.../quorum/FollowerRequestProcessor.java | 1 +
.../quorum/ObserverRequestProcessor.java | 1 +
.../quorum/ReadOnlyRequestProcessor.java | 1 +
.../util/RequestPathMetricsCollector.java | 1 +
.../org/apache/zookeeper/test/CheckTest.java | 13 +++---------
9 files changed, 16 insertions(+), 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
index d3e20eca205..911583f9fc2 100644
--- 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
index bcbfbaad73c..9bf6c7eea56 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -806,6 +806,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
index 27fa4e2dff7..c174fdd1e2f 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java
@@ -295,8 +295,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:
@@ -352,6 +352,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
index 0445bbdefff..3be43c37e30 100644
--- 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/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
index 2798dd789d2..ef01ab32242 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
+++ b/zookeeper-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/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java
index 565f2778b1d..1d7e5c5c37e 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java
+++ b/zookeeper-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/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java
index 7824af5cb54..671d328ff43 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java
+++ b/zookeeper-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/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java
index 6163872510f..db6f8c56645 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java
+++ b/zookeeper-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
index e813132739a..0e2a1a4eca4 100644
--- 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);