aweisberg commented on code in PR #74:
URL: https://github.com/apache/cassandra-accord/pull/74#discussion_r1412458587
##########
accord-core/src/main/java/accord/messages/Request.java:
##########
@@ -23,7 +23,16 @@
public interface Request extends Message
{
- void process(Node on, Id from, ReplyContext replyContext);
default long waitForEpoch() { return 0; }
default long knownEpoch() { return waitForEpoch(); }
+ void preProcess(Node on, Id from, ReplyContext replyContext);
+ void process(Node on, Id from, ReplyContext replyContext);
+
+ /**
+ * Process the request without replying back
+ */
+ default void process(Node on)
Review Comment:
So many processes makes this a little messy and it's spreading to other
classes implementing `no-op` methods.
Since propagate is the only class that does `process(Node on)` and it goes
to a dedicated `LocalRequest.Hander` could that not be part of `Request`?
I don't know what `preProcess` and `process` being split is for, I imagine
it is clear on the integration side.
##########
accord-core/src/main/java/accord/messages/Propagate.java:
##########
@@ -212,6 +224,18 @@ else if (achieved.deps.hasProposedOrDecidedDeps())
return Keys.EMPTY;
}
+ @Override
+ public void preProcess(Node on, Node.Id from, ReplyContext replyContext)
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void process(Node on, Node.Id from, ReplyContext replyContext)
+ {
+ throw new UnsupportedOperationException();
Review Comment:
Heh, some dance is going on here with the various versions of process I just
haven't figured it out yet.
##########
accord-core/src/main/java/accord/messages/MessageType.java:
##########
@@ -98,25 +101,40 @@ public enum Kind { LOCAL, REMOTE }
values = builder.build();
}
- protected static MessageType mt(Kind kind, boolean hasSideEffects)
+ protected static MessageType local(String name, boolean hasSideEffects)
{
- return new MessageType(kind, hasSideEffects);
+ return new MessageType(name, LOCAL, hasSideEffects);
}
- /**
- * LOCAL messages are not sent to remote nodes.
- */
+ protected static MessageType remote(String name, boolean hasSideEffects)
+ {
+ return new MessageType(name, REMOTE, hasSideEffects);
+ }
+
+ private final String name;
Review Comment:
FYI Benedict wanted to change this back to an enum again have multiple enums
that share a common interface so the integrations can add more types.
##########
accord-core/src/main/java/accord/messages/LocalRequest.java:
##########
@@ -19,13 +19,19 @@
import accord.local.Node;
import accord.local.PreLoadContext;
+import accord.local.SafeCommandStore;
+import accord.utils.MapReduceConsume;
-public interface LocalMessage extends Message, PreLoadContext
+import java.util.function.BiConsumer;
+
+public interface LocalRequest<R> extends Request, PreLoadContext,
MapReduceConsume<SafeCommandStore, Void>
{
+ void process(Node on, BiConsumer<R, Throwable> callback);
+
+ BiConsumer<R, Throwable> callback();
+
interface Handler
{
- void handle(LocalMessage message, Node node);
+ void handle(LocalRequest<?> message, Node node);
}
-
- void process(Node node);
Review Comment:
This is weird because we call this now from `process(Node on,
BiConsumer<Status.Known, Throwable> callback)` so why have both?
--
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]
