This is an automated email from the ASF dual-hosted git repository.
jxue pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 0da9b4245 Delete expected version (#2759)
0da9b4245 is described below
commit 0da9b42452b80db64e4698d091f21fe421086e5e
Author: Grant Paláu Spencer <[email protected]>
AuthorDate: Tue Mar 19 15:51:29 2024 -0700
Delete expected version (#2759)
Add delete with expected version API to zk client and data accessor
---
.../java/org/apache/helix/BaseDataAccessor.java | 11 +++++++
.../helix/manager/zk/ZkBaseDataAccessor.java | 21 ++++++++++++
.../helix/manager/zk/ZkCacheBaseDataAccessor.java | 25 +++++++++++++++
.../helix/store/zk/AutoFallbackPropertyStore.java | 8 +++++
.../helix/manager/zk/TestZkBaseDataAccessor.java | 37 ++++++++++++++++++++++
.../apache/helix/mock/MockBaseDataAccessor.java | 18 +++++++++++
.../zookeeper/api/client/RealmAwareZkClient.java | 2 ++
.../zookeeper/impl/client/DedicatedZkClient.java | 7 +++-
.../zookeeper/impl/client/FederatedZkClient.java | 7 +++-
.../zookeeper/impl/client/SharedZkClient.java | 7 +++-
.../helix/zookeeper/zkclient/IZkConnection.java | 2 ++
.../apache/helix/zookeeper/zkclient/ZkClient.java | 14 +++++++-
.../helix/zookeeper/zkclient/ZkConnection.java | 5 +++
.../client/RealmAwareZkClientFactoryTestBase.java | 24 ++++++++++++++
.../impl/client/TestFederatedZkClient.java | 26 ++++++++++++++-
15 files changed, 209 insertions(+), 5 deletions(-)
diff --git a/helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java
b/helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java
index 162ffb05d..4fafb4271 100644
--- a/helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java
+++ b/helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java
@@ -112,6 +112,17 @@ public interface BaseDataAccessor<T> {
*/
boolean remove(String path, int options);
+ /**
+ * This will remove the ZNode, if the ZNode's version matches the provided
expectedVersion. This
+ * operation will fail if the node has any children.
+ * @param path Path to the ZNode to update
+ * @param options Set the type of ZNode see the valid values in {@link
AccessOption}
+ * @param expectedVersion the expected version of the node to be removed, -1
means match any
+ * version
+ * @return true if the removal succeeded, false otherwise
+ */
+ boolean removeWithExpectedVersion(String path, int options, int
expectedVersion);
+
/**
* Use it when creating children under a parent node. This will use async
api for better
* performance. If the child already exists it will return false.
diff --git
a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
index e074a22e6..06ea97fb9 100644
---
a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
+++
b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
@@ -788,6 +788,27 @@ public class ZkBaseDataAccessor<T> implements
BaseDataAccessor<T> {
return true;
}
+ /**
+ * Sync remove with expected version. It tries to remove the ZNode if the
ZNode's version matches
+ * the provided expectedVersion. This operation will FAIL if the node has
any children. Node does
+ * not exist is regarded as success. If expectedVersion is set to -1, then
the ZNode version
+ * match is not enforced.
+ */
+ @Override
+ public boolean removeWithExpectedVersion(String path, int options, int
expectedVersion) {
+ try {
+ // operation will not throw exception when path successfully deleted or
does not exist
+ // despite real error, operation will throw exception when path not
empty, and in this
+ // case, we do NOT try to delete recursively
+ _zkClient.delete(path, expectedVersion);
+ } catch (ZkException zkException) {
+ LOG.error("Failed to delete {} with opts {} and expected version {}.",
path, options,
+ expectedVersion, zkException);
+ return false;
+ }
+ return true;
+ }
+
/**
* async create. give up on error other than NONODE
*/
diff --git
a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
index 6a635a5da..2be1a4f76 100644
---
a/helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
+++
b/helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
@@ -356,6 +356,31 @@ public class ZkCacheBaseDataAccessor<T> implements
HelixPropertyStore<T> {
return _baseAccessor.remove(serverPath, options);
}
+ @Override
+ public boolean removeWithExpectedVersion(String path, int options, int
expectedVersion) {
+ String clientPath = path;
+ String serverPath = prependChroot(clientPath);
+
+ Cache<T> cache = getCache(serverPath);
+ if (cache != null) {
+ try {
+ cache.lockWrite();
+
+ boolean success = _baseAccessor.removeWithExpectedVersion(serverPath,
options, expectedVersion);
+ if (success) {
+ cache.purgeRecursive(serverPath);
+ }
+
+ return success;
+ } finally {
+ cache.unlockWrite();
+ }
+ }
+
+ // no cache
+ return _baseAccessor.removeWithExpectedVersion(serverPath, options,
expectedVersion);
+ }
+
@Override
public T get(String path, Stat stat, int options) {
String clientPath = path;
diff --git
a/helix-core/src/main/java/org/apache/helix/store/zk/AutoFallbackPropertyStore.java
b/helix-core/src/main/java/org/apache/helix/store/zk/AutoFallbackPropertyStore.java
index c23bdca90..ce82206a6 100644
---
a/helix-core/src/main/java/org/apache/helix/store/zk/AutoFallbackPropertyStore.java
+++
b/helix-core/src/main/java/org/apache/helix/store/zk/AutoFallbackPropertyStore.java
@@ -95,6 +95,14 @@ public class AutoFallbackPropertyStore<T> extends
ZkHelixPropertyStore<T> {
return super.remove(path, options);
}
+ @Override
+ public boolean removeWithExpectedVersion(String path, int options, int
expectedVersion) {
+ if (_fallbackStore != null) {
+ _fallbackStore.removeWithExpectedVersion(path, options, expectedVersion);
+ }
+ return super.removeWithExpectedVersion(path, options, expectedVersion);
+ }
+
@Override
public T get(String path, Stat stat, int options) {
if (_fallbackStore == null) {
diff --git
a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java
b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java
index bf4f342dd..56337f547 100644
---
a/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java
+++
b/helix-core/src/test/java/org/apache/helix/manager/zk/TestZkBaseDataAccessor.java
@@ -543,6 +543,43 @@ public class TestZkBaseDataAccessor extends ZkUnitTestBase
{
System.out.println("END " + testName + " at " + new
Date(System.currentTimeMillis()));
}
+ /**
+ * Test that remove with expected version will fail on version mismatch.
Succeed on version match.
+ */
+ @Test
+ public void testRemoveWithExpectedVersion() {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String testName = className + "_" + methodName;
+
+ System.out.println("START " + testName + " at " + new
Date(System.currentTimeMillis()));
+
+ String path = String.format("/%s/%s", _rootPath, "msg_0");
+ ZNRecord record = new ZNRecord("msg_0");
+ ZkBaseDataAccessor<ZNRecord> accessor = new
ZkBaseDataAccessor<ZNRecord>(_gZkClient);
+
+ // Create node
+ Assert.assertTrue(accessor.create(path, record, AccessOption.PERSISTENT));
+
+ // Create child node
+ String childPath = path + "/child";
+ Assert.assertTrue(accessor.create(childPath, record,
AccessOption.PERSISTENT));
+
+ // Delete parent with correct expected version. Should fail due to having
a child
+ int currentVersion = accessor.getStat(path, 0).getVersion();
+ Assert.assertFalse(accessor.removeWithExpectedVersion(path, 0,
currentVersion));
+
+ // Remove Child
+ Assert.assertTrue(accessor.removeWithExpectedVersion(childPath, 0, -1));
+
+ // Delete childless node with wrong expected version. Should fail due to
version mismatch
+ Assert.assertFalse(accessor.removeWithExpectedVersion(path, 0,
currentVersion+100));
+
+ // Delete childless node with correct expected version. Shoudl succeed
+ Assert.assertTrue(accessor.removeWithExpectedVersion(path, 0,
currentVersion));
+
+ }
+
@Test
public void testDeleteNodeWithChildren() {
String root = _rootPath;
diff --git
a/helix-core/src/test/java/org/apache/helix/mock/MockBaseDataAccessor.java
b/helix-core/src/test/java/org/apache/helix/mock/MockBaseDataAccessor.java
index 1567b98cc..da5418d00 100644
--- a/helix-core/src/test/java/org/apache/helix/mock/MockBaseDataAccessor.java
+++ b/helix-core/src/test/java/org/apache/helix/mock/MockBaseDataAccessor.java
@@ -111,6 +111,24 @@ public class MockBaseDataAccessor implements
BaseDataAccessor<ZNRecord> {
return true;
}
+ @Override
+ public boolean removeWithExpectedVersion(String path, int options, int
expectedVersion) {
+ if (expectedVersion != -1) {
+ ZNode node = _recordMap.get(path);
+ if (node != null && node.getStat().getVersion() != expectedVersion) {
+ return false;
+ }
+ }
+
+ _recordMap.remove(path);
+ try {
+ Thread.sleep(50);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ return true;
+ }
+
@Override
public boolean[] createChildren(List<String> paths, List<ZNRecord> records,
int options) {
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
index ac0240d5f..e24cc6e9c 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
@@ -254,6 +254,8 @@ public interface RealmAwareZkClient {
boolean delete(final String path);
+ boolean delete(final String path, final int expectedVersion);
+
<T extends Object> T readData(String path);
<T extends Object> T readData(String path, boolean
returnNullIfPathNotExists);
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
index 8dba7d81e..a1a06f48d 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/DedicatedZkClient.java
@@ -385,8 +385,13 @@ public class DedicatedZkClient implements
RealmAwareZkClient {
@Override
public boolean delete(String path) {
+ return delete(path, -1);
+ }
+
+ @Override
+ public boolean delete(String path, int expectedVersion) {
checkIfPathContainsShardingKey(path);
- return _rawZkClient.delete(path);
+ return _rawZkClient.delete(path, expectedVersion);
}
@Override
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java
index 8069985e3..219ad9703 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/FederatedZkClient.java
@@ -372,7 +372,12 @@ public class FederatedZkClient implements
RealmAwareZkClient {
@Override
public boolean delete(String path) {
- return getZkClient(path).delete(path);
+ return delete(path, -1);
+ }
+
+ @Override
+ public boolean delete(String path, int expectedVersion) {
+ return getZkClient(path).delete(path, expectedVersion);
}
@Override
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
index 093538fa6..367628868 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/client/SharedZkClient.java
@@ -414,8 +414,13 @@ public class SharedZkClient implements RealmAwareZkClient {
@Override
public boolean delete(String path) {
+ return delete(path, -1);
+ }
+
+ @Override
+ public boolean delete(String path, int expectedVersion) {
checkIfPathContainsShardingKey(path);
- return _innerSharedZkClient.delete(path);
+ return _innerSharedZkClient.delete(path, expectedVersion);
}
@Override
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkConnection.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkConnection.java
index 43eefad26..30dee9468 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkConnection.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/IZkConnection.java
@@ -45,6 +45,8 @@ public interface IZkConnection {
public void delete(String path) throws InterruptedException,
KeeperException;
+ public void delete (String path, int expectedVersion) throws
InterruptedException, KeeperException;
+
boolean exists(final String path, final boolean watch) throws
KeeperException, InterruptedException;
List<String> getChildren(final String path, final boolean watch) throws
KeeperException, InterruptedException;
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
index 110587e09..9d21e7604 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
@@ -2158,6 +2158,18 @@ public class ZkClient implements Watcher {
* @return true if path is successfully deleted, false if path does not exist
*/
public boolean delete(final String path) {
+ return delete(path, -1);
+ }
+
+ /**
+ * Delete the given path. Path should not have any children or the deletion
will fail.
+ * This function will throw an exception if we fail to delete an existing
path.
+ * @param path Path of the znode to delete
+ * @param expectedVersion The expected version of the node to be removed, -1
means match any
+ * version. ZK
+ * @return true if the path is successfully deleted, false if path does not
exist
+ */
+ public boolean delete(final String path, final int expectedVersion) {
long startT = System.currentTimeMillis();
boolean success;
try {
@@ -2166,7 +2178,7 @@ public class ZkClient implements Watcher {
@Override
public Object call() throws Exception {
- getConnection().delete(path);
+ getConnection().delete(path, expectedVersion);
return null;
}
});
diff --git
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
index 376409231..07d01df61 100644
---
a/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
+++
b/zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkConnection.java
@@ -145,6 +145,11 @@ public class ZkConnection implements IZkConnection {
_zk.delete(path, -1);
}
+ @Override
+ public void delete(String path, int expectedVersion) throws
InterruptedException, KeeperException {
+ _zk.delete(path, expectedVersion);
+ }
+
@Override
public boolean exists(String path, boolean watch) throws KeeperException,
InterruptedException {
return _zk.exists(path, watch) != null;
diff --git
a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java
b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java
index 29c2df0f8..bae31de40 100644
---
a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java
+++
b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/RealmAwareZkClientFactoryTestBase.java
@@ -26,6 +26,7 @@ import
org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.zkclient.exception.ZkBadVersionException;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
@@ -238,4 +239,27 @@ public abstract class RealmAwareZkClientFactoryTestBase
extends RealmAwareZkClie
Assert.assertTrue(_realmAwareZkClient.delete(TEST_VALID_PATH));
Assert.assertFalse(_realmAwareZkClient.exists(TEST_VALID_PATH));
}
+
+ /**
+ * Test that delete() with expected version fails on version mismatch.
Succeeds on version match.
+ */
+ @Test(dependsOnMethods = "testDelete")
+ public void testDeleteExpectedVersion() {
+ // Create a ZNode for testing
+ _realmAwareZkClient.createPersistent(TEST_VALID_PATH, true);
+ Assert.assertTrue(_realmAwareZkClient.exists(TEST_VALID_PATH));
+ int expectedVersion =
_realmAwareZkClient.getStat(TEST_VALID_PATH).getVersion();
+
+ // Test delete with invalid version, should fail to delete
+ try {
+ _realmAwareZkClient.delete(TEST_VALID_PATH, expectedVersion + 100);
+ Assert.fail("Should have thrown bad version exception");
+ } catch (ZkBadVersionException expectedException) {
+ // Expected exception, continue
+ }
+
+ // Assert delete with expected version successful
+ Assert.assertTrue(_realmAwareZkClient.delete(TEST_VALID_PATH,
expectedVersion));
+ Assert.assertFalse(_realmAwareZkClient.exists(TEST_VALID_PATH));
+ }
}
diff --git
a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestFederatedZkClient.java
b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestFederatedZkClient.java
index 30335efb1..3fea6a8bf 100644
---
a/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestFederatedZkClient.java
+++
b/zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestFederatedZkClient.java
@@ -40,6 +40,7 @@ import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
import org.apache.helix.zookeeper.routing.RoutingDataManager;
import org.apache.helix.zookeeper.zkclient.IZkStateListener;
+import org.apache.helix.zookeeper.zkclient.exception.ZkBadVersionException;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.Watcher;
import org.testng.Assert;
@@ -289,10 +290,33 @@ public class TestFederatedZkClient extends
RealmAwareZkClientTestBase {
Assert.assertFalse(_realmAwareZkClient.exists(TEST_REALM_ONE_VALID_PATH));
}
+ /**
+ * Tests that delete() works on version match and fails on version mismatch
+ */
+ @Test(dependsOnMethods = "testDelete")
+ public void testDeleteExpectedVersion() {
+ // Create a ZNode for testing
+ _realmAwareZkClient.createPersistent(TEST_VALID_PATH, true);
+ Assert.assertTrue(_realmAwareZkClient.exists(TEST_VALID_PATH));
+ int expectedVersion =
_realmAwareZkClient.getStat(TEST_VALID_PATH).getVersion();
+
+ // Test delete with invalid version, should fail to delete
+ try {
+ _realmAwareZkClient.delete(TEST_VALID_PATH, expectedVersion + 100);
+ Assert.fail("Should hvae thrown bad version exception");
+ } catch (ZkBadVersionException expectedException) {
+ // Expected exception, continue
+ }
+
+ // Assert delete with expected version successful
+ Assert.assertTrue(_realmAwareZkClient.delete(TEST_VALID_PATH,
expectedVersion));
+ Assert.assertFalse(_realmAwareZkClient.exists(TEST_VALID_PATH));
+ }
+
/*
* Tests that multi-realm feature.
*/
- @Test(dependsOnMethods = "testDelete")
+ @Test(dependsOnMethods = "testDeleteExpectedVersion")
public void testMultiRealmCRUD() {
ZNRecord realmOneZnRecord = new ZNRecord("realmOne");
realmOneZnRecord.setSimpleField("realmOne", "Value");
