You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2023/01/09 17:31:41 UTC

[GitHub] [helix] Marcosrico opened a new pull request, #2336: Op and OpResult Logic

Marcosrico opened a new pull request, #2336:
URL: https://github.com/apache/helix/pull/2336

   ### Issues
   
   - [ ] My PR addresses the following Helix issues and references them in the PR description:
   
   (#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
   Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
   
   ### Description
   
   - [ ] Here are some details about my PR, including screenshots of any UI changes:
   
   (Write a concise description including what, why, how)
   
   ### Tests
   
   - [ ] The following tests are written for this issue:
   
   (List the names of added unit/integration tests)
   
   - The following is the result of the "mvn test" command on the appropriate module:
   
   (If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
   
   ### Changes that Break Backward Compatibility (Optional)
   
   - My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
   
   (Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
   
   ### Documentation (Optional)
   
   - In case of new functionality, my PR adds documentation in the following wiki page:
   
   (Link the GitHub wiki you added)
   
   ### Commits
   
   - My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
     1. Subject is separated from body by a blank line
     1. Subject is limited to 50 characters (not including Jira issue reference)
     1. Subject does not end with a period
     1. Subject uses the imperative mood ("add", not "adding")
     1. Body wraps at 72 characters
     1. Body explains "what" and "why", not "how"
   
   ### Code Quality
   
   - My diff has been formatted using helix-style.xml 
   (helix-style-intellij.xml if IntelliJ IDE is used)
   


-- 
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


[GitHub] [helix] junkaixue commented on pull request #2336: Op and OpResult Logic

Posted by GitBox <gi...@apache.org>.
junkaixue commented on PR #2336:
URL: https://github.com/apache/helix/pull/2336#issuecomment-1376010611

   We need more concrete description on PR description. It will be hard for us to understand it at high level


-- 
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


[GitHub] [helix] Marcosrico commented on a diff in pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1066181731


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;
+
+  private Op(int type, String path) {
+    this.type = type;
+    this.path = path;
+  }
+  public static Op create(String path, byte[] data) {
+    return new Create(path, data);
+  }
+
+  public static Op create(String path, byte[] data, MetaClientInterface.EntryMode createMode) {
+    return new Create(path, data, createMode);
+  }
+
+  public static Op delete(String path, int version) {
+    return new Op.Delete(path, version);
+  }
+
+  public static Op set(String path, byte[] data, int version) {
+    return new Set(path, data, version);
+  }
+
+  public static Op check(String path, int version) {
+    return new Check(path, version);
+  }
+
+  public int getType() {
+    return this.type;
+  }

Review Comment:
   I agree, will change from int to enum. Thanks!



-- 
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


[GitHub] [helix] Marcosrico commented on pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on PR #2336:
URL: https://github.com/apache/helix/pull/2336#issuecomment-1386081904

   This PR is ready to merge, approved by @qqu0127
   Commit message:
   Metaclient - Op and OpResult Logic for transactional support


-- 
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


[GitHub] [helix] xyuanlu merged pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
xyuanlu merged PR #2336:
URL: https://github.com/apache/helix/pull/2336


-- 
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


[GitHub] [helix] Marcosrico commented on a diff in pull request #2336: Op and OpResult Logic

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1065129303


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/OpResult.java:
##########
@@ -19,8 +19,131 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
 /**
  * Represent the result of a single operation of a multi operation transaction.
  */
 public class OpResult {
-}
+  private int type;
+
+  private OpResult(int type) {
+      this.type = type;
+  }
+
+  public int getType() {
+      return this.type;
+  }
+
+  public static class ErrorResult extends OpResult {
+    private int err;
+
+    public ErrorResult(int err) {
+      super(-1);
+      this.err = err;
+    }
+
+    public int getErr() {
+        return this.err;
+    }
+  }
+
+  public static class GetDataResult extends OpResult {
+    private byte[] data;
+    private MetaClientInterface.Stat stat;
+
+    public GetDataResult(byte[] data, MetaClientInterface.Stat stat) {
+      super(4);
+      this.data = data == null ? null : Arrays.copyOf(data, data.length);
+      this.stat = stat;
+    }
+
+    public byte[] getData() {
+      return this.data == null ? null : Arrays.copyOf(this.data, this.data.length);
+    }
+
+    public MetaClientInterface.Stat getStat() {
+      return this.stat;
+    }
+  }
+
+  public static class GetChildrenResult extends OpResult {
+    private List<String> children;
+
+    public GetChildrenResult(List<String> children) {
+      super(8);
+      this.children = children;
+    }
+
+    public List<String> getChildren() {
+        return this.children;
+    }
+  }
+
+  public static class CheckResult extends OpResult {
+    public CheckResult() {
+      super(13);
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      } else if (!(o instanceof CheckResult)) {
+        return false;
+      } else {
+        CheckResult other = (CheckResult)o;
+        return this.getType() == other.getType();
+      }
+    }

Review Comment:
   Sorry my apologies. I was referencing Zk OpResult but misunderstood that metaclient, as of now, does not need these methods. Thanks for the catch!



##########
meta-client/src/main/java/org/apache/helix/metaclient/api/OpResult.java:
##########
@@ -19,8 +19,131 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
 /**
  * Represent the result of a single operation of a multi operation transaction.
  */
 public class OpResult {
-}
+  private int type;
+
+  private OpResult(int type) {
+      this.type = type;
+  }
+
+  public int getType() {
+      return this.type;
+  }
+
+  public static class ErrorResult extends OpResult {
+    private int err;
+
+    public ErrorResult(int err) {
+      super(-1);
+      this.err = err;
+    }
+
+    public int getErr() {
+        return this.err;
+    }
+  }
+
+  public static class GetDataResult extends OpResult {
+    private byte[] data;
+    private MetaClientInterface.Stat stat;
+
+    public GetDataResult(byte[] data, MetaClientInterface.Stat stat) {
+      super(4);
+      this.data = data == null ? null : Arrays.copyOf(data, data.length);
+      this.stat = stat;
+    }
+
+    public byte[] getData() {
+      return this.data == null ? null : Arrays.copyOf(this.data, this.data.length);
+    }
+
+    public MetaClientInterface.Stat getStat() {
+      return this.stat;
+    }
+  }
+
+  public static class GetChildrenResult extends OpResult {
+    private List<String> children;
+
+    public GetChildrenResult(List<String> children) {
+      super(8);
+      this.children = children;
+    }
+
+    public List<String> getChildren() {
+        return this.children;
+    }
+  }
+
+  public static class CheckResult extends OpResult {
+    public CheckResult() {
+      super(13);
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      } else if (!(o instanceof CheckResult)) {
+        return false;
+      } else {
+        CheckResult other = (CheckResult)o;
+        return this.getType() == other.getType();
+      }
+    }

Review Comment:
   My apologies. I was referencing Zk OpResult but misunderstood that metaclient, as of now, does not need these methods. Thanks for the catch!



-- 
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


[GitHub] [helix] Marcosrico commented on pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on PR #2336:
URL: https://github.com/apache/helix/pull/2336#issuecomment-1386086151

   This PR is ready to merge, approved by @qqu0127
   Commit message:
   Metaclient - Op and OpResult Logic for transactional support


-- 
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


[GitHub] [helix] junkaixue commented on a diff in pull request #2336: Op and OpResult Logic

Posted by GitBox <gi...@apache.org>.
junkaixue commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1064917483


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;
+
+  private Op(int type, String path) {
+    this.type = type;
+    this.path = path;
+  }
+  public static Op create(String path, byte[] data) {
+    return new Create(path, data);

Review Comment:
   A little bit confused there. Is this circular call? Op is the abstract class try to create object for Create, which is super class for Op.
   
   Could you please give more explanation here?
   
   Better we have java doc for those APIs.



-- 
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


[GitHub] [helix] qqu0127 commented on a diff in pull request #2336: Op and OpResult Logic

Posted by GitBox <gi...@apache.org>.
qqu0127 commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1064907198


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;

Review Comment:
   please follow the coding style, we usually name member variable with _type



##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;
+
+  private Op(int type, String path) {
+    this.type = type;
+    this.path = path;
+  }
+  public static Op create(String path, byte[] data) {
+    return new Create(path, data);
+  }
+
+  public static Op create(String path, byte[] data, MetaClientInterface.EntryMode createMode) {
+    return new Create(path, data, createMode);
+  }
+
+  public static Op delete(String path, int version) {
+    return new Op.Delete(path, version);
+  }
+
+  public static Op set(String path, byte[] data, int version) {
+    return new Set(path, data, version);
+  }
+
+  public static Op check(String path, int version) {
+    return new Check(path, version);
+  }
+
+  public int getType() {
+    return this.type;
+  }

Review Comment:
   People might be confused by this. 
   Plain integer type is also prone to error, especially for future edit, one suggestion is to use enum instead.



##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;
+
+  private Op(int type, String path) {
+    this.type = type;
+    this.path = path;
+  }
+  public static Op create(String path, byte[] data) {
+    return new Create(path, data);
+  }
+
+  public static Op create(String path, byte[] data, MetaClientInterface.EntryMode createMode) {
+    return new Create(path, data, createMode);
+  }
+
+  public static Op delete(String path, int version) {
+    return new Op.Delete(path, version);
+  }
+
+  public static Op set(String path, byte[] data, int version) {
+    return new Set(path, data, version);
+  }
+
+  public static Op check(String path, int version) {
+    return new Check(path, version);
+  }
+
+  public int getType() {
+    return this.type;
+  }
+
+  public String getPath() {
+    return this.path;
+  }
+
   /**
    * Check the version of an entry. True only when the version is the same as expected.
    */
   public static class Check extends Op {
+    private final int version;
+    public int getVersion() { return version;}
+    private Check(String path, int version) {
+      super(13, path);
+      this.version = version;
+    }
   }
-  public static class Create extends Op{
+  public static class Create extends Op {
+    protected final byte[] data;
+    private MetaClientInterface.EntryMode mode;
+
+    public byte[] getData() {
+      return data;
+    }
+    public MetaClientInterface.EntryMode getEntryMode() {return mode;}
+
+    private Create(String path, byte[] data) {
+      super(1, path);

Review Comment:
   Magic number here, maybe another reason to use enum :D 
   Same for the following



##########
meta-client/src/main/java/org/apache/helix/metaclient/api/OpResult.java:
##########
@@ -19,8 +19,131 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
 /**
  * Represent the result of a single operation of a multi operation transaction.
  */
 public class OpResult {
-}
+  private int type;
+
+  private OpResult(int type) {
+      this.type = type;
+  }
+
+  public int getType() {
+      return this.type;
+  }
+
+  public static class ErrorResult extends OpResult {
+    private int err;
+
+    public ErrorResult(int err) {
+      super(-1);
+      this.err = err;
+    }
+
+    public int getErr() {
+        return this.err;
+    }
+  }
+
+  public static class GetDataResult extends OpResult {
+    private byte[] data;
+    private MetaClientInterface.Stat stat;
+
+    public GetDataResult(byte[] data, MetaClientInterface.Stat stat) {
+      super(4);
+      this.data = data == null ? null : Arrays.copyOf(data, data.length);
+      this.stat = stat;
+    }
+
+    public byte[] getData() {
+      return this.data == null ? null : Arrays.copyOf(this.data, this.data.length);
+    }
+
+    public MetaClientInterface.Stat getStat() {
+      return this.stat;
+    }
+  }
+
+  public static class GetChildrenResult extends OpResult {
+    private List<String> children;
+
+    public GetChildrenResult(List<String> children) {
+      super(8);
+      this.children = children;
+    }
+
+    public List<String> getChildren() {
+        return this.children;
+    }
+  }
+
+  public static class CheckResult extends OpResult {
+    public CheckResult() {
+      super(13);
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      } else if (!(o instanceof CheckResult)) {
+        return false;
+      } else {
+        CheckResult other = (CheckResult)o;
+        return this.getType() == other.getType();
+      }
+    }

Review Comment:
   We only compare  getType()??
   Does this mean every instance are the same?



##########
meta-client/src/main/java/org/apache/helix/metaclient/api/OpResult.java:
##########
@@ -19,8 +19,131 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
 /**
  * Represent the result of a single operation of a multi operation transaction.
  */
 public class OpResult {
-}
+  private int type;
+
+  private OpResult(int type) {
+      this.type = type;
+  }
+
+  public int getType() {
+      return this.type;
+  }
+
+  public static class ErrorResult extends OpResult {
+    private int err;
+
+    public ErrorResult(int err) {
+      super(-1);
+      this.err = err;
+    }
+
+    public int getErr() {
+        return this.err;
+    }
+  }
+
+  public static class GetDataResult extends OpResult {
+    private byte[] data;
+    private MetaClientInterface.Stat stat;
+
+    public GetDataResult(byte[] data, MetaClientInterface.Stat stat) {
+      super(4);
+      this.data = data == null ? null : Arrays.copyOf(data, data.length);
+      this.stat = stat;
+    }
+
+    public byte[] getData() {
+      return this.data == null ? null : Arrays.copyOf(this.data, this.data.length);
+    }
+
+    public MetaClientInterface.Stat getStat() {
+      return this.stat;
+    }
+  }
+
+  public static class GetChildrenResult extends OpResult {
+    private List<String> children;
+
+    public GetChildrenResult(List<String> children) {
+      super(8);
+      this.children = children;
+    }
+
+    public List<String> getChildren() {
+        return this.children;
+    }
+  }
+
+  public static class CheckResult extends OpResult {
+    public CheckResult() {
+      super(13);
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      } else if (!(o instanceof CheckResult)) {
+        return false;
+      } else {
+        CheckResult other = (CheckResult)o;
+        return this.getType() == other.getType();
+      }
+    }
+
+    public int hashCode() {
+      return this.getType();
+    }

Review Comment:
   why do we need this and above?



-- 
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


[GitHub] [helix] Marcosrico commented on a diff in pull request #2336: Op and OpResult Logic

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1065129496


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/OpResult.java:
##########
@@ -19,8 +19,131 @@
  * under the License.
  */
 
+import java.util.Arrays;
+import java.util.List;
 /**
  * Represent the result of a single operation of a multi operation transaction.
  */
 public class OpResult {
-}
+  private int type;
+
+  private OpResult(int type) {
+      this.type = type;
+  }
+
+  public int getType() {
+      return this.type;
+  }
+
+  public static class ErrorResult extends OpResult {
+    private int err;
+
+    public ErrorResult(int err) {
+      super(-1);
+      this.err = err;
+    }
+
+    public int getErr() {
+        return this.err;
+    }
+  }
+
+  public static class GetDataResult extends OpResult {
+    private byte[] data;
+    private MetaClientInterface.Stat stat;
+
+    public GetDataResult(byte[] data, MetaClientInterface.Stat stat) {
+      super(4);
+      this.data = data == null ? null : Arrays.copyOf(data, data.length);
+      this.stat = stat;
+    }
+
+    public byte[] getData() {
+      return this.data == null ? null : Arrays.copyOf(this.data, this.data.length);
+    }
+
+    public MetaClientInterface.Stat getStat() {
+      return this.stat;
+    }
+  }
+
+  public static class GetChildrenResult extends OpResult {
+    private List<String> children;
+
+    public GetChildrenResult(List<String> children) {
+      super(8);
+      this.children = children;
+    }
+
+    public List<String> getChildren() {
+        return this.children;
+    }
+  }
+
+  public static class CheckResult extends OpResult {
+    public CheckResult() {
+      super(13);
+    }
+
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      } else if (!(o instanceof CheckResult)) {
+        return false;
+      } else {
+        CheckResult other = (CheckResult)o;
+        return this.getType() == other.getType();
+      }
+    }
+
+    public int hashCode() {
+      return this.getType();
+    }

Review Comment:
   See above comment



-- 
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


[GitHub] [helix] Marcosrico commented on a diff in pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
Marcosrico commented on code in PR #2336:
URL: https://github.com/apache/helix/pull/2336#discussion_r1067240859


##########
meta-client/src/main/java/org/apache/helix/metaclient/api/Op.java:
##########
@@ -23,16 +23,96 @@
  *  Represents a single operation in a multi-operation transaction.  Each operation can be a create, set,
  *  version check or delete operation.
  */
-public class Op {
+public abstract class Op {
+  private int type;
+  private String path;
+
+  private Op(int type, String path) {
+    this.type = type;
+    this.path = path;
+  }
+  public static Op create(String path, byte[] data) {
+    return new Create(path, data);

Review Comment:
   There might be a slight circular call though it is not cyclic. Op creates a Create object which calls its super's (Op) constructor to set the type and path. The implementation of the Op (and OpResult) objects are based on zkclient's implementation of such. It'll be easier for users to adopt metaclient if api resembles that of zkclient.



-- 
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


[GitHub] [helix] xyuanlu commented on pull request #2336: MetaClient - Op and OpResult Logic for transactional support

Posted by GitBox <gi...@apache.org>.
xyuanlu commented on PR #2336:
URL: https://github.com/apache/helix/pull/2336#issuecomment-1380791668

   Generally looks good. Please add JavaDoc.


-- 
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