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: [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]
