[GitHub] [helix] qqu0127 commented on a diff in pull request #2327: Meta client - implement CRUD for zkMetaClient

[GitBox <gi...@apache.org>]

qqu0127 commented on code in PR #2327:
URL: https://github.com/apache/helix/pull/2327#discussion_r1061893029


##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -49,55 +61,97 @@ public ZkMetaClient(ZkMetaClientConfig config) {
   }
 
   @Override
-  public void create(String key, Object data) {
-
+  public void create(String key, T data) {
+    // TODO: This function is implemented only for test. It does not have proper error handling
+    _zkClient.create(key, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
   }
 
   @Override
-  public void create(String key, Object data, EntryMode mode) {
+  public void create(String key, T data, EntryMode mode) {
 
   }
 
   @Override
-  public void set(String key, Object data, int version) {
-
+  public void set(String key, T data, int version) {
+    try {
+      _zkClient.writeData(key, data, version);
+    } catch (ZkException e) {
+      throw translateZkExceptionToMetaclientException(e);
+    }
   }
 
   @Override
-  public Object update(String key, DataUpdater updater) {
-    return null;
+  public T update( String key, DataUpdater<T> updater) {
+    org.apache.zookeeper.data.Stat stat = new org.apache.zookeeper.data.Stat();
+    // TODO: add retry logic for ZkBadVersionException.
+    try {
+      T oldData = _zkClient.readData(key, stat);
+      T newData = updater.update(oldData);
+      set(key, newData, stat.getVersion());
+      return newData;
+    } catch (ZkException e) {
+      throw translateZkExceptionToMetaclientException(e);
+    }
   }
 
   @Override
   public Stat exists(String key) {
-    return null;
+    org.apache.zookeeper.data.Stat zkStats;
+    try {
+      zkStats = _zkClient.getStat(key);
+      if (zkStats == null) {
+        return null;
+      }
+      return new Stat(EphemeralType.get(zkStats.getEphemeralOwner()) == EphemeralType.VOID
+          ? EntryMode.PERSISTENT : EntryMode.EPHEMERAL, zkStats.getVersion());
+    } catch (ZkException e) {
+      throw translateZkExceptionToMetaclientException(e);
+    }
   }
 
   @Override
-  public Object get(String key) {
-    return null;
+  public T get(String key) {
+    return _zkClient.readData(key, true);
   }
 
   @Override
-  public List<String> getDirestChildrenKeys(String key) {
+  public List<OpResult> transactionOP(Iterable<Op> ops) {
     return null;
   }
 
   @Override
-  public int countDirestChildren(String key) {
-    return 0;
+  public List<String> getDirectChildrenKeys(String key) {
+    try {
+      return _zkClient.getChildren(key);
+    } catch (ZkException e) {
+      throw new MetaClientException(e);
+    }
+  }
+
+  @Override
+  public int countDirectChildren(String key) {
+    return _zkClient.countChildren(key);
   }
 
   @Override
   public boolean delete(String key) {
-    return false;
+    try {
+      return _zkClient.delete(key);
+    } catch (ZkException e) {
+      throw new MetaClientException(e);
+    }
   }
 
   @Override
   public boolean recursiveDelete(String key) {
-    return false;
+    _zkClient.deleteRecursively(key);

Review Comment:
   Just curious, can we have a return value from `deleteRecursively` ? 



##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientException.java:
##########
@@ -19,5 +19,20 @@
  * under the License.
  */
 
-public final class MetaClientException {
-}
+public class MetaClientException extends RuntimeException{

Review Comment:
   nit: space before the last {



##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -155,12 +209,15 @@ public void asyncTransaction(Iterable iterable, AsyncCallback.TransactionCallbac
 
   @Override
   public boolean connect() {
-    return false;
+    // TODO: This is a tempp impl for test only. no proper event handling and error handling.
+    _zkClient.connect(Integer.MAX_VALUE, _zkClient);
+    return true;

Review Comment:
   For my own understanding, does this work with the current test?
   ( I remember during my own test, it fails if the zkclient has already connected, which is the case in native zkclient as it's connected during initialization.)



##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -41,8 +53,125 @@ public void prepare() {
   }
 
   @Test
-  public void dummyTest() {
-    Assert.assertEquals(1+1, 2);
+  public void testGet() {
+    final String key = "/TestZkMetaClient_testGet";
+    ZkMetaClient<String> zkMetaClient = createZkMetaClient();
+    String value;
+    zkMetaClient.create(key, ENTRY_STRING_VALUE);
+    String dataValue = zkMetaClient.get(key);
+    Assert.assertEquals(dataValue, ENTRY_STRING_VALUE);
+
+    value = zkMetaClient.get(key + "/a/b/c");
+    Assert.assertNull(value);
+
+    zkMetaClient.delete(key);
+
+    value = zkMetaClient.get(key);
+    Assert.assertNull(value);

Review Comment:
   Should we close metaclient to avoid leakage in test? Maybe we can wrap the test in a try-with-resource block.



##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientBadVersionException.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.helix.metaclient.constants;
+
+public class MetaClientBadVersionException extends MetaClientException {
+  public MetaClientBadVersionException() {
+    super();
+  }

Review Comment:
   Same for others, do we really need such empty constructor?



##########
meta-client/src/test/java/org/apache/helix/metaclient/impl/zk/TestZkMetaClient.java:
##########
@@ -41,8 +53,125 @@ public void prepare() {
   }
 
   @Test
-  public void dummyTest() {
-    Assert.assertEquals(1+1, 2);
+  public void testGet() {
+    final String key = "/TestZkMetaClient_testGet";
+    ZkMetaClient<String> zkMetaClient = createZkMetaClient();
+    String value;
+    zkMetaClient.create(key, ENTRY_STRING_VALUE);

Review Comment:
   Following the connect() comment above, after the full implementation, metaclient should call connect() before any operation, right? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org