rahulrane50 commented on code in PR #2327:
URL: https://github.com/apache/helix/pull/2327#discussion_r1066171216
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -30,76 +30,133 @@
import org.apache.helix.metaclient.api.DirectChildChangeListener;
import org.apache.helix.metaclient.api.DirectChildSubscribeResult;
import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.api.Op;
import org.apache.helix.metaclient.api.OpResult;
+import org.apache.helix.metaclient.constants.MetaClientBadVersionException;
+import org.apache.helix.metaclient.constants.MetaClientException;
+import org.apache.helix.metaclient.constants.MetaClientInterruptException;
+import org.apache.helix.metaclient.constants.MetaClientNoNodeException;
+import org.apache.helix.metaclient.constants.MetaClientTimeoutException;
import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
import org.apache.helix.zookeeper.impl.client.ZkClient;
import org.apache.helix.zookeeper.zkclient.IZkDataListener;
import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.exception.ZkBadVersionException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNodeExistsException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkTimeoutException;
+import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.server.EphemeralType;
-public class ZkMetaClient implements MetaClientInterface {
+public class ZkMetaClient<T> implements MetaClientInterface<T>, AutoCloseable {
private final ZkClient _zkClient;
+ private final int _connectionTimeout;
public ZkMetaClient(ZkMetaClientConfig config) {
- _zkClient = new ZkClient(new ZkConnection(config.getConnectionAddress(),
- (int) config.getSessionTimeoutInMillis()),
- (int) config.getConnectionInitTimeoutInMillis(), -1
/*operationRetryTimeout*/,
- config.getZkSerializer(), config.getMonitorType(),
config.getMonitorKey(),
- config.getMonitorInstanceName(), config.getMonitorRootPathOnly());
+ _connectionTimeout = (int) config.getConnectionInitTimeoutInMillis();
+ _zkClient = new ZkClient(
+ new ZkConnection(config.getConnectionAddress(), (int)
config.getSessionTimeoutInMillis()),
+ _connectionTimeout, -1 /*operationRetryTimeout*/,
config.getZkSerializer(),
+ config.getMonitorType(), config.getMonitorKey(),
config.getMonitorInstanceName(),
+ config.getMonitorRootPathOnly(), false);
}
@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) {
Review Comment:
shouldn't this T extend an Object class?
##########
meta-client/src/main/java/org/apache/helix/metaclient/api/MetaClientInterface.java:
##########
@@ -332,9 +352,14 @@ void asyncCreate(final String key, final T data, final
EntryMode mode,
/**
* Maintains a connection with underlying metadata service based on config
params. Connection
* created by this method will be used to perform CRUD operations on
metadata service.
- * @return True if connection is successfully established.
- */
- boolean connect();
Review Comment:
Any reason we changed return type from boolean to void?
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -30,76 +30,133 @@
import org.apache.helix.metaclient.api.DirectChildChangeListener;
import org.apache.helix.metaclient.api.DirectChildSubscribeResult;
import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.api.Op;
import org.apache.helix.metaclient.api.OpResult;
+import org.apache.helix.metaclient.constants.MetaClientBadVersionException;
+import org.apache.helix.metaclient.constants.MetaClientException;
+import org.apache.helix.metaclient.constants.MetaClientInterruptException;
+import org.apache.helix.metaclient.constants.MetaClientNoNodeException;
+import org.apache.helix.metaclient.constants.MetaClientTimeoutException;
import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
import org.apache.helix.zookeeper.impl.client.ZkClient;
import org.apache.helix.zookeeper.zkclient.IZkDataListener;
import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.exception.ZkBadVersionException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNodeExistsException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkTimeoutException;
+import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.server.EphemeralType;
-public class ZkMetaClient implements MetaClientInterface {
+public class ZkMetaClient<T> implements MetaClientInterface<T>, AutoCloseable {
private final ZkClient _zkClient;
+ private final int _connectionTimeout;
public ZkMetaClient(ZkMetaClientConfig config) {
- _zkClient = new ZkClient(new ZkConnection(config.getConnectionAddress(),
- (int) config.getSessionTimeoutInMillis()),
- (int) config.getConnectionInitTimeoutInMillis(), -1
/*operationRetryTimeout*/,
- config.getZkSerializer(), config.getMonitorType(),
config.getMonitorKey(),
- config.getMonitorInstanceName(), config.getMonitorRootPathOnly());
+ _connectionTimeout = (int) config.getConnectionInitTimeoutInMillis();
+ _zkClient = new ZkClient(
+ new ZkConnection(config.getConnectionAddress(), (int)
config.getSessionTimeoutInMillis()),
+ _connectionTimeout, -1 /*operationRetryTimeout*/,
config.getZkSerializer(),
+ config.getMonitorType(), config.getMonitorKey(),
config.getMonitorInstanceName(),
+ config.getMonitorRootPathOnly(), false);
}
@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) {
Review Comment:
Umm now I see implementation of "exists" i think more about is it "check if
this key exists" API or it's more like "get this key if exists". Implementation
suggests later case then i think name could be different.
##########
meta-client/src/main/java/org/apache/helix/metaclient/constants/MetaClientBadVersionException.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.helix.metaclient.constants;
+
+public final class MetaClientBadVersionException extends MetaClientException {
Review Comment:
@qqu0127 we can add something like apache rat in CI pipeline for this right?
We can make sure that all new files will be having all necessary info.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -30,76 +30,133 @@
import org.apache.helix.metaclient.api.DirectChildChangeListener;
import org.apache.helix.metaclient.api.DirectChildSubscribeResult;
import org.apache.helix.metaclient.api.MetaClientInterface;
+import org.apache.helix.metaclient.api.Op;
import org.apache.helix.metaclient.api.OpResult;
+import org.apache.helix.metaclient.constants.MetaClientBadVersionException;
+import org.apache.helix.metaclient.constants.MetaClientException;
+import org.apache.helix.metaclient.constants.MetaClientInterruptException;
+import org.apache.helix.metaclient.constants.MetaClientNoNodeException;
+import org.apache.helix.metaclient.constants.MetaClientTimeoutException;
import org.apache.helix.metaclient.impl.zk.factory.ZkMetaClientConfig;
import org.apache.helix.zookeeper.impl.client.ZkClient;
import org.apache.helix.zookeeper.zkclient.IZkDataListener;
import org.apache.helix.zookeeper.zkclient.ZkConnection;
+import org.apache.helix.zookeeper.zkclient.exception.ZkBadVersionException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkInterruptedException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkNodeExistsException;
+import org.apache.helix.zookeeper.zkclient.exception.ZkTimeoutException;
+import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.server.EphemeralType;
-public class ZkMetaClient implements MetaClientInterface {
+public class ZkMetaClient<T> implements MetaClientInterface<T>, AutoCloseable {
private final ZkClient _zkClient;
+ private final int _connectionTimeout;
public ZkMetaClient(ZkMetaClientConfig config) {
- _zkClient = new ZkClient(new ZkConnection(config.getConnectionAddress(),
- (int) config.getSessionTimeoutInMillis()),
- (int) config.getConnectionInitTimeoutInMillis(), -1
/*operationRetryTimeout*/,
- config.getZkSerializer(), config.getMonitorType(),
config.getMonitorKey(),
- config.getMonitorInstanceName(), config.getMonitorRootPathOnly());
+ _connectionTimeout = (int) config.getConnectionInitTimeoutInMillis();
+ _zkClient = new ZkClient(
+ new ZkConnection(config.getConnectionAddress(), (int)
config.getSessionTimeoutInMillis()),
+ _connectionTimeout, -1 /*operationRetryTimeout*/,
config.getZkSerializer(),
+ config.getMonitorType(), config.getMonitorKey(),
config.getMonitorInstanceName(),
+ config.getMonitorRootPathOnly(), false);
}
@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) {
Review Comment:
This comment at all other places as well.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
