qqu0127 commented on code in PR #2343:
URL: https://github.com/apache/helix/pull/2343#discussion_r1081444524
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -50,11 +51,15 @@
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.server.EphemeralType;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
public class ZkMetaClient<T> implements MetaClientInterface<T>, AutoCloseable {
private final ZkClient _zkClient;
+ //Default ACL value until metaClient Op has ACL of its own.
+ private final List<ACL> DEFAULT_ACL = ZooDefs.Ids.OPEN_ACL_UNSAFE;
Review Comment:
This can be static? Also we can use ImmutableList as a wrapper around this.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -414,4 +414,130 @@ private static EntryMode convertZkEntryMode(long
ephemeralOwner) {
throw new IllegalArgumentException(zkEphemeralType + " is not
supported.");
}
}
+
+
+ @Override
+ public List<OpResult> transactionOP(Iterable<Op> iterable) throws
KeeperException {
+ // Convert list of MetaClient Ops to Zk Ops
+ List<org.apache.zookeeper.Op> zkOps = metaClientOpToZk(iterable);
+ // Execute Zk transactional support
+ List<org.apache.zookeeper.OpResult> zkResult = _zkClient.multi(zkOps);
+ // Convert list of Zk OpResults to MetaClient OpResults
+ return zkOpResultToMetaClient(zkResult);
+ }
+
+ /**
+ * Helper function for transactionOp. Converts MetaClient Op's into Zk Ops
to execute
+ * zk transactional support.
+ * @param ops
+ * @return
+ */
+ public List<org.apache.zookeeper.Op> metaClientOpToZk(Iterable<Op> ops)
throws KeeperException {
+ List<org.apache.zookeeper.Op> zkOps = new ArrayList<>();
+ org.apache.zookeeper.Op temp;
+ for (Op op : ops) {
+ switch (op.getType()) {
+ case CREATE:
+ int zkFlag = zkFlagFromEntryMode(((Op.Create) op).getEntryMode());
+ temp = org.apache.zookeeper.Op.create(
+ op.getPath(), ((Op.Create) op).getData(), DEFAULT_ACL,
CreateMode.fromFlag(zkFlag));
+ break;
+ case DELETE:
+ temp = org.apache.zookeeper.Op.delete(
+ op.getPath(), ((Op.Delete) op).getVersion());
+ break;
+ case SET:
+ temp = org.apache.zookeeper.Op.setData(
+ op.getPath(), ((Op.Set) op).getData(), ((Op.Set)
op).getVersion());
+ break;
+ case CHECK:
+ temp = org.apache.zookeeper.Op.check(
+ op.getPath(), ((Op.Check) op).getVersion());
+ break;
+ default:
+ throw new IllegalArgumentException(op.getType() + " is not
supported.");
+ }
+ zkOps.add(temp);
+ }
+ return zkOps;
+ }
+
+ /**
+ * Helper function for transactionOP. Converts the result from calling zk
transactional support into
+ * metaclient OpResults.
+ * @param zkResult
+ * @return
+ */
+ public List<OpResult>
zkOpResultToMetaClient(List<org.apache.zookeeper.OpResult> zkResult) throws
KeeperException.BadArgumentsException {
+ List<OpResult> metaClientOpResult = new ArrayList<>();
+ OpResult temp;
+ EntryMode zkMode;
+ for (org.apache.zookeeper.OpResult opResult : zkResult) {
+ Stat metaClientStat;
+ switch (opResult.getType()) {
+ // CreateResult
+ case 1:
+ temp = new OpResult.CreateResult(
+ ((org.apache.zookeeper.OpResult.CreateResult)
opResult).getPath());
+ break;
+ // DeleteResult
+ case 2:
+ temp = new OpResult.DeleteResult();
+ break;
+ // GetDataResult
+ case 4:
+ org.apache.zookeeper.OpResult.GetDataResult zkOpGetDataResult =
+ (org.apache.zookeeper.OpResult.GetDataResult) opResult;
+ metaClientStat = new
Stat(convertZkEntryMode(zkOpGetDataResult.getStat().getEphemeralOwner()),
+ zkOpGetDataResult.getStat().getVersion());
+ temp = new OpResult.GetDataResult(zkOpGetDataResult.getData(),
metaClientStat);
+ break;
+ //SetDataResult
+ case 5:
+ org.apache.zookeeper.OpResult.SetDataResult zkOpSetDataResult =
+ (org.apache.zookeeper.OpResult.SetDataResult) opResult;
+ metaClientStat = new
Stat(convertZkEntryMode(zkOpSetDataResult.getStat().getEphemeralOwner()),
+ zkOpSetDataResult.getStat().getVersion());
+ temp = new OpResult.SetDataResult(metaClientStat);
+ break;
+ //GetChildrenResult
+ case 8:
+ temp = new OpResult.GetChildrenResult(
+ ((org.apache.zookeeper.OpResult.GetChildrenResult)
opResult).getChildren());
+ break;
+ //CheckResult
+ case 13:
+ temp = new OpResult.CheckResult();
+ break;
+ //CreateResult with stat
+ case 15:
+ org.apache.zookeeper.OpResult.CreateResult zkOpCreateResult =
+ (org.apache.zookeeper.OpResult.CreateResult) opResult;
+ metaClientStat = new
Stat(convertZkEntryMode(zkOpCreateResult.getStat().getEphemeralOwner()),
+ zkOpCreateResult.getStat().getVersion());
+ temp = new OpResult.CreateResult(zkOpCreateResult.getPath(),
metaClientStat);
+ break;
+ //ErrorResult
+ case -1:
+ temp = new OpResult.ErrorResult(
+ ((org.apache.zookeeper.OpResult.ErrorResult)
opResult).getErr());
+ break;
+ default:
+ throw new IllegalArgumentException(opResult.getType() + " is not
supported.");
+ }
+ metaClientOpResult.add(temp);
+ }
+ return metaClientOpResult;
+ }
+
+ private int zkFlagFromEntryMode (EntryMode entryMode) {
+ String mode = entryMode.name();
+ if (mode.equals(EntryMode.PERSISTENT.name())) {
+ return 0;
+ }
+ if (mode.equals(EntryMode.EPHEMERAL.name())) {
+ return 1;
+ }
+ return -1;
+ }
Review Comment:
If I understand correctly, in the end, what you are trying to achieve is
translate entryMode to ZK.CreateMode. Both of them are enums, let's try to
bridge them in one pass, without the "flag" as intermediate param.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -414,4 +415,128 @@ private static EntryMode convertZkEntryMode(long
ephemeralOwner) {
throw new IllegalArgumentException(zkEphemeralType + " is not
supported.");
}
}
+
+
+ @Override
+ public List<OpResult> transactionOP(Iterable<Op> iterable) {
+ // Convert list of MetaClient Ops to Zk Ops
+ List<org.apache.zookeeper.Op> zkOps = metaClientOpToZk(iterable);
+ // Execute Zk transactional support
+ List<org.apache.zookeeper.OpResult> zkResult = _zkClient.multi(zkOps);
+ // Convert list of Zk OpResults to MetaClient OpResults
+ return zkOpResultToMetaClient(zkResult);
+ }
+
+ /**
+ * Helper function for transactionOp. Converts MetaClient Op's into Zk Ops
to execute
+ * zk transactional support.
+ * @param ops
+ * @return
+ */
+ public List<org.apache.zookeeper.Op> metaClientOpToZk(Iterable<Op> ops) {
+ List<org.apache.zookeeper.Op> zkOps = new ArrayList<>();
+ org.apache.zookeeper.Op temp;
+ for (Op op : ops) {
+ switch (op.getType()) {
+ case CREATE:
+ int zkFlag = zkFlagFromEntryMode(((Op.Create) op).getEntryMode());
+ try {
+ temp = org.apache.zookeeper.Op.create(
+ op.getPath(), ((Op.Create) op).getData(), DEFAULT_ACL,
CreateMode.fromFlag(zkFlag));
+ } catch (KeeperException e) {
+ throw new MetaClientKeeperException(e);
+ }
+ break;
+ case DELETE:
+ temp = org.apache.zookeeper.Op.delete(
+ op.getPath(), ((Op.Delete) op).getVersion());
+ break;
+ case SET:
+ temp = org.apache.zookeeper.Op.setData(
+ op.getPath(), ((Op.Set) op).getData(), ((Op.Set)
op).getVersion());
+ break;
+ case CHECK:
+ temp = org.apache.zookeeper.Op.check(
+ op.getPath(), ((Op.Check) op).getVersion());
+ break;
+ default:
+ throw new IllegalArgumentException(op.getType() + " is not
supported.");
+ }
+ zkOps.add(temp);
+ }
+ return zkOps;
+ }
+
+ /**
+ * Helper function for transactionOP. Converts the result from calling zk
transactional support into
+ * metaclient OpResults.
+ * @param zkResult
+ * @return
+ */
+ public List<OpResult>
zkOpResultToMetaClient(List<org.apache.zookeeper.OpResult> zkResult) {
Review Comment:
Overall suggestion, for all helper functions like this, let's move to a
standalone class
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -414,4 +415,128 @@ private static EntryMode convertZkEntryMode(long
ephemeralOwner) {
throw new IllegalArgumentException(zkEphemeralType + " is not
supported.");
}
}
+
+
+ @Override
+ public List<OpResult> transactionOP(Iterable<Op> iterable) {
+ // Convert list of MetaClient Ops to Zk Ops
+ List<org.apache.zookeeper.Op> zkOps = metaClientOpToZk(iterable);
+ // Execute Zk transactional support
+ List<org.apache.zookeeper.OpResult> zkResult = _zkClient.multi(zkOps);
+ // Convert list of Zk OpResults to MetaClient OpResults
+ return zkOpResultToMetaClient(zkResult);
+ }
+
+ /**
+ * Helper function for transactionOp. Converts MetaClient Op's into Zk Ops
to execute
+ * zk transactional support.
+ * @param ops
+ * @return
+ */
+ public List<org.apache.zookeeper.Op> metaClientOpToZk(Iterable<Op> ops) {
+ List<org.apache.zookeeper.Op> zkOps = new ArrayList<>();
+ org.apache.zookeeper.Op temp;
+ for (Op op : ops) {
+ switch (op.getType()) {
+ case CREATE:
+ int zkFlag = zkFlagFromEntryMode(((Op.Create) op).getEntryMode());
+ try {
+ temp = org.apache.zookeeper.Op.create(
+ op.getPath(), ((Op.Create) op).getData(), DEFAULT_ACL,
CreateMode.fromFlag(zkFlag));
+ } catch (KeeperException e) {
+ throw new MetaClientKeeperException(e);
+ }
+ break;
+ case DELETE:
+ temp = org.apache.zookeeper.Op.delete(
+ op.getPath(), ((Op.Delete) op).getVersion());
+ break;
+ case SET:
+ temp = org.apache.zookeeper.Op.setData(
+ op.getPath(), ((Op.Set) op).getData(), ((Op.Set)
op).getVersion());
+ break;
+ case CHECK:
+ temp = org.apache.zookeeper.Op.check(
+ op.getPath(), ((Op.Check) op).getVersion());
+ break;
+ default:
+ throw new IllegalArgumentException(op.getType() + " is not
supported.");
+ }
+ zkOps.add(temp);
+ }
+ return zkOps;
+ }
+
+ /**
+ * Helper function for transactionOP. Converts the result from calling zk
transactional support into
+ * metaclient OpResults.
+ * @param zkResult
+ * @return
+ */
+ public List<OpResult>
zkOpResultToMetaClient(List<org.apache.zookeeper.OpResult> zkResult) {
+ List<OpResult> metaClientOpResult = new ArrayList<>();
+ OpResult temp;
+ String resultClass;
+ for (org.apache.zookeeper.OpResult opResult : zkResult) {
+ Stat metaClientStat;
+ resultClass = opResult.getClass().getSimpleName();
+ switch (resultClass) {
+ case "CreateResult":
Review Comment:
Comparing on classname is too risky and low on performance (it's still magic
number to some extent). If you must do it this way, please check `instance of`.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -414,4 +414,130 @@ private static EntryMode convertZkEntryMode(long
ephemeralOwner) {
throw new IllegalArgumentException(zkEphemeralType + " is not
supported.");
}
}
+
+
+ @Override
+ public List<OpResult> transactionOP(Iterable<Op> iterable) throws
KeeperException {
+ // Convert list of MetaClient Ops to Zk Ops
+ List<org.apache.zookeeper.Op> zkOps = metaClientOpToZk(iterable);
+ // Execute Zk transactional support
+ List<org.apache.zookeeper.OpResult> zkResult = _zkClient.multi(zkOps);
+ // Convert list of Zk OpResults to MetaClient OpResults
+ return zkOpResultToMetaClient(zkResult);
+ }
+
+ /**
+ * Helper function for transactionOp. Converts MetaClient Op's into Zk Ops
to execute
+ * zk transactional support.
+ * @param ops
+ * @return
+ */
+ public List<org.apache.zookeeper.Op> metaClientOpToZk(Iterable<Op> ops)
throws KeeperException {
Review Comment:
Does this have to be public?
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java:
##########
@@ -414,4 +415,128 @@ private static EntryMode convertZkEntryMode(long
ephemeralOwner) {
throw new IllegalArgumentException(zkEphemeralType + " is not
supported.");
}
}
+
+
+ @Override
+ public List<OpResult> transactionOP(Iterable<Op> iterable) {
+ // Convert list of MetaClient Ops to Zk Ops
+ List<org.apache.zookeeper.Op> zkOps = metaClientOpToZk(iterable);
+ // Execute Zk transactional support
+ List<org.apache.zookeeper.OpResult> zkResult = _zkClient.multi(zkOps);
+ // Convert list of Zk OpResults to MetaClient OpResults
+ return zkOpResultToMetaClient(zkResult);
+ }
+
+ /**
+ * Helper function for transactionOp. Converts MetaClient Op's into Zk Ops
to execute
+ * zk transactional support.
+ * @param ops
+ * @return
+ */
+ public List<org.apache.zookeeper.Op> metaClientOpToZk(Iterable<Op> ops) {
+ List<org.apache.zookeeper.Op> zkOps = new ArrayList<>();
+ org.apache.zookeeper.Op temp;
+ for (Op op : ops) {
+ switch (op.getType()) {
+ case CREATE:
+ int zkFlag = zkFlagFromEntryMode(((Op.Create) op).getEntryMode());
+ try {
+ temp = org.apache.zookeeper.Op.create(
+ op.getPath(), ((Op.Create) op).getData(), DEFAULT_ACL,
CreateMode.fromFlag(zkFlag));
+ } catch (KeeperException e) {
+ throw new MetaClientKeeperException(e);
+ }
+ break;
+ case DELETE:
+ temp = org.apache.zookeeper.Op.delete(
+ op.getPath(), ((Op.Delete) op).getVersion());
+ break;
+ case SET:
+ temp = org.apache.zookeeper.Op.setData(
+ op.getPath(), ((Op.Set) op).getData(), ((Op.Set)
op).getVersion());
+ break;
+ case CHECK:
+ temp = org.apache.zookeeper.Op.check(
+ op.getPath(), ((Op.Check) op).getVersion());
+ break;
+ default:
+ throw new IllegalArgumentException(op.getType() + " is not
supported.");
+ }
+ zkOps.add(temp);
+ }
+ return zkOps;
+ }
+
+ /**
+ * Helper function for transactionOP. Converts the result from calling zk
transactional support into
+ * metaclient OpResults.
+ * @param zkResult
+ * @return
+ */
+ public List<OpResult>
zkOpResultToMetaClient(List<org.apache.zookeeper.OpResult> zkResult) {
+ List<OpResult> metaClientOpResult = new ArrayList<>();
+ OpResult temp;
+ String resultClass;
+ for (org.apache.zookeeper.OpResult opResult : zkResult) {
+ Stat metaClientStat;
+ resultClass = opResult.getClass().getSimpleName();
+ switch (resultClass) {
+ case "CreateResult":
Review Comment:
I see the difficulties here in binding two classes without knowing the type
in compile time. One potential way to improve similar code, is to try using a
static map to store the binding and utilize lambda functional.
e.g.
```
// mapping from class object => construction function of OpResult
Map<Class<? extends org.apache.zookeeper.OpResult>,
Function<org.apache.zookeeper.OpResult, OpResult>> map = new HashMap<>();
map.put(org.apache.zookeeper.OpResult.CheckResult.class, op -> new
OpResult.CheckResult());
for (org.apache.zookeeper.OpResult opResult : zkResult) {
map.get(opResult.getClass()).apply(opResult);
}
```
--
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]
