rpuch commented on code in PR #1518:
URL: https://github.com/apache/ignite-3/pull/1518#discussion_r1068984477
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/Operation.java:
##########
@@ -17,158 +17,90 @@
package org.apache.ignite.internal.metastorage.dsl;
-import org.jetbrains.annotations.NotNull;
+import org.apache.ignite.internal.util.IgniteUtils;
import org.jetbrains.annotations.Nullable;
/**
* Defines operation for meta storage conditional update (invoke).
*/
public final class Operation {
- /** Actual operation implementation. */
- private final InnerOp upd;
-
/**
- * Constructs an operation which wraps the actual operation implementation.
- *
- * @param upd The actual operation implementation.
+ * Key identifies an entry which operation will be applied to. Key is
{@code null} for {@link OperationType#NO_OP} operation.
*/
- Operation(InnerOp upd) {
- this.upd = upd;
- }
+ private final byte @Nullable [] key;
/**
- * Returns actual operation implementation.
+ * Value which will be associated with the {@link #key}. Value is not
{@code null} only for {@link OperationType#PUT} operation.
*/
- public InnerOp inner() {
- return upd;
- }
+ private final byte @Nullable [] val;
/**
- * Returns operation type.
+ * Operation type.
*
- * @return Operation type.
+ * @see OperationType
*/
- public OperationType type() {
- return upd.type();
- }
+ private final OperationType type;
/**
- * Represents operation of type <i>remove</i>.
+ * Constructs operation which will be applied to an entry identified by
the given key.
+ *
+ * @param type Operation type. Can't be {@code null}.
+ * @param key Key identifies an entry which operation will be applied to.
+ * @param val Value will be associated with an entry identified by the
{@code key}.
*/
- public static final class RemoveOp extends AbstractOp {
- /**
- * Default no-op constructor.
- *
- * @param key Identifies an entry which operation will be applied to.
- */
- RemoveOp(byte[] key) {
- super(key, OperationType.REMOVE);
- }
+ public Operation(OperationType type, byte @Nullable [] key, byte @Nullable
[] val) {
+ assert (type == OperationType.NO_OP && key == null && val == null)
+ || (type == OperationType.PUT && key != null && val != null)
+ || (type == OperationType.REMOVE && key != null && val == null)
+ : "Invalid operation parameters: [type=" + type
+ + ", key=" + (key == null ? "null" :
IgniteUtils.toHexString(key, 256))
+ + ", val=" + (val == null ? "null" :
IgniteUtils.toHexString(val, 256)) + ']';
+
+ this.key = key;
+ this.val = val;
+ this.type = type;
}
/**
- * Represents operation of type <i>put</i>.
+ * Returns a key which identifies an entry which operation will be applied
to.
+ *
+ * @return A key which identifies an entry which operation will be applied
to.
*/
- public static final class PutOp extends AbstractOp {
- /** Value. */
- private final byte[] val;
-
- /**
- * Constructs operation of type <i>put</i>.
- *
- * @param key Identifies an entry which operation will be applied to.
- * @param val The value to which the entry should be updated.
- */
- PutOp(byte[] key, byte[] val) {
- super(key, OperationType.PUT);
-
- this.val = val;
- }
-
- /**
- * Returns value.
- *
- * @return Value.
- */
- public byte[] value() {
- return val;
- }
+ public byte @Nullable [] key() {
+ return key;
}
/**
- * Represents operation of type <i>no-op</i>.
+ * Returns a value which will be associated with an entry identified by
the {@code key}.
+ *
+ * @return A value which will be associated with an entry identified by
the {@code key}.
*/
- public static final class NoOp extends AbstractOp {
- /**
- * Default no-op constructor.
- */
- NoOp() {
- super(null, OperationType.NO_OP);
- }
+
+ public byte @Nullable [] value() {
+ return val;
}
/**
- * Defines operation interface.
+ * Returns an operation type.
+ *
+ * @return An operation type.
*/
- public interface InnerOp {
- /**
- * Returns key.
- *
- * @return Key.
- */
- @Nullable byte[] key();
-
- /**
- * Returns operation type.
- *
- * @return Operation type.
- */
- @NotNull OperationType type();
+ public OperationType type() {
+ return type;
}
/**
- * Extension of {@link InnerOp}.
+ * Represents operation of type <i>remove</i>.
*/
- private static class AbstractOp implements InnerOp {
- /** Key. */
- @Nullable
- private final byte[] key;
-
- /** Operation type. */
- @NotNull
- private final OperationType type;
-
- /**
- * Ctor.
- *
- * @param key Key.
- * @param type Operation type.
- */
- private AbstractOp(@Nullable byte[] key, OperationType type) {
- this.key = key;
- this.type = type;
- }
+ public static Operation remove(byte[] key) {
+ return new Operation(OperationType.REMOVE, key, null);
+ }
- /**
- * Returns key.
- *
- * @return Key.
- */
- @Nullable
- @Override
- public byte[] key() {
- return key;
- }
+ public static Operation put(byte[] key, byte[] val) {
Review Comment:
`put()` and `noOp()` do not have any javadoc, when `remove()` does have one.
Is this intentional?
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/SimpleCondition.java:
##########
@@ -20,46 +20,82 @@
/**
* Represents a condition for a meta storage conditional update.
*/
-public final class SimpleCondition implements Condition {
- /** Actual condition implementation. */
- private final InnerCondition cond;
+public class SimpleCondition implements Condition {
+ /** Entry key. */
+ private final byte[] key;
/**
- * Constructs a condition, which wraps the actual condition implementation.
- *
- * @param cond The actual condition implementation.
+ * Condition type.
*/
- SimpleCondition(InnerCondition cond) {
- this.cond = cond;
+ private final ConditionType type;
+
+ private SimpleCondition(byte[] key, ConditionType type) {
+ this.key = key;
+ this.type = type;
}
- public InnerCondition inner() {
- return cond;
+ /**
+ * Returns the key, which identifies an entry, which condition will be
applied to.
+ *
+ * @return Key, which identifies an entry, which condition will be applied
to.
+ */
+ public byte[] key() {
+ return key;
}
public ConditionType type() {
- return cond.type();
+ return type;
+ }
+
+ public static RevisionConditionBuilder revision(byte[] key) {
Review Comment:
How about a javadoc?
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/watch/WatchAggregatorTest.java:
##########
@@ -274,8 +274,8 @@ private Entry entry(String key, String val, long revision,
long updateCntr) {
return new Entry() {
/** {@inheritDoc} */
@Override
- public @NotNull ByteArray key() {
- return new ByteArray(key);
+ public @NotNull byte[] key() {
Review Comment:
Let's remove `@NotNull`
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/SimpleCondition.java:
##########
@@ -304,155 +277,31 @@ public SimpleCondition lt(byte[] val) {
* @throws IllegalStateException In the case when the condition is
already defined.
*/
public SimpleCondition le(byte[] val) {
- validate(type());
-
- type(ConditionType.VAL_LESS_OR_EQUAL);
-
- this.val = val;
-
- return new SimpleCondition(this);
+ return new ValueCondition(key, ConditionType.VAL_LESS_OR_EQUAL,
val);
}
}
/**
- * Represents a condition on an entry existence. Only the one type of a
condition could be applied to the one instance of a condition.
+ * Represents a condition on an entry value. Only the one type of
condition could be applied to the one instance of a condition.
Review Comment:
```suggestion
* Represents a condition on an entry value. Only one type of condition
could be applied to one instance of a condition.
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -160,11 +163,11 @@ public void close() {
// Meta Storage should not return nulls as values
assert value != null;
- if (key.equals(MASTER_KEY)) {
+ if (Arrays.equals(key, MASTER_KEY.bytes())) {
continue;
}
- String dataKey =
key.toString().substring(DISTRIBUTED_PREFIX.length());
+ String dataKey = new String(key,
StandardCharsets.UTF_8).substring(DISTRIBUTED_PREFIX.length());
Review Comment:
`UTF_8` can be imported statically decreasing the amount of syntactic noise:
it is not that important that it goes from `StandardCharsets`; the important
piece of information here is that it's utf-8.
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/Entry.java:
##########
@@ -37,7 +35,7 @@ public interface Entry {
*
* @return The key.
*/
- @NotNull ByteArray key();
+ byte[] key();
Review Comment:
Why was `ByteArray` changed to `byte[]` here? `ByteArray` seemed to only add
equals/comparison support; isn't it useful anymore?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/EntryImpl.java:
##########
@@ -19,17 +19,32 @@
import java.util.Arrays;
import org.apache.ignite.internal.metastorage.Entry;
-import org.apache.ignite.lang.ByteArray;
-import org.jetbrains.annotations.NotNull;
+import org.apache.ignite.internal.tostring.S;
import org.jetbrains.annotations.Nullable;
/**
- * Meta storage entry.
+ * Represents a storage unit as entry with key, value and revision.
+ *
+ * <p>Where:
+ * <ul>
+ * <li>key - an unique entry's key represented by an array of bytes. Keys
are comparable in lexicographic manner.</li>
+ * <li>value - a data which is associated with a key and represented as an
array of bytes.</li>
+ * <li>revision - a number which denotes a version of whole meta storage.
+ * Each change (which could include multiple entries) increments the
revision. </li>
+ * <li>updateCounter - a number which increments on every update in the
change under one revision.</li>
+ * </ul>
+ *
+ * <p>Instance of {@link #EntryImpl} could represent:
+ * <ul>
+ * <li>A regular entry which stores a particular key, a value and a
revision number.</li>
+ * <li>An empty entry which denotes absence a regular entry in the meta
storage for a given key.
Review Comment:
```suggestion
* <li>An empty entry which denotes absence of a regular entry in the
meta storage for a given key.
```
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/dsl/SimpleCondition.java:
##########
@@ -171,37 +167,44 @@ public SimpleCondition lt(long rev) {
* @throws IllegalStateException In the case when the condition is
already defined.
*/
public SimpleCondition le(long rev) {
- assert rev > 0 : "Revision must be positive.";
-
- validate(type());
-
- type(ConditionType.REV_LESS_OR_EQUAL);
-
- this.rev = rev;
-
- return new SimpleCondition(this);
+ return new RevisionCondition(key, ConditionType.REV_LESS_OR_EQUAL,
rev);
}
}
/**
- * Represents a condition on an entry value. Only the one type of
condition could be applied to the one instance of a condition.
+ * Represents a condition on an entry revision. Only one type of condition
could be applied to the one instance of a condition.
Review Comment:
```suggestion
* Represents a condition on an entry revision. Only one type of
condition could be applied to one instance of a condition.
```
--
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]
