belliottsmith commented on code in PR #10:
URL: https://github.com/apache/cassandra-accord/pull/10#discussion_r988569766
##########
accord-core/src/main/java/accord/messages/Apply.java:
##########
@@ -54,15 +63,59 @@ public Apply(Node.Id to, Topologies topologies, TxnId
txnId, Txn txn, Key homeKe
this.result = result;
}
+ @VisibleForImplementation
+ public Apply(Keys scope, long waitForEpoch, TxnId txnId, Txn txn, Key
homeKey, Timestamp executeAt, Deps deps, Writes writes, Result result)
+ {
+ super(scope, waitForEpoch);
+ this.txnId = txnId;
+ this.txn = txn;
+ this.homeKey = homeKey;
+ this.executeAt = executeAt;
+ this.deps = deps;
+ this.writes = writes;
+ this.result = result;
+ }
+
+ static Future<Void> waitAndReduce(Future<Void> left, Future<Void> right)
Review Comment:
> You mean a detailed and informed discussion of the pros and cons of
different solutions? I feel like the project could use more of this ;)
No, absolutely :)
> In the case where we interact with a single command store, futures
actually use slightly less memory than callbacks.
That definitely means we've done callbacks wrong :) ... bear in mind, the
`Future` has to allocate a list entry for the callback, potentially allocates a
`WaitQueue` for consumers like those above, and it has to perform a CAS to add
the callbacks, signal the callbacks, and to set the result (also at least four
for each waiting `Thread`). At _most_ a callback should have to do the last of
these (and usually not even that).
> Here’s the callback class if you’d like to take a look
This looks like it is trying to implement a map/Reduce chain over a list of
callbacks, i.e. the `waitAndReduce` method - rather than a single callback? But
we have to solve this anyway still above, as we shouldn't be blocking, I think.
So we will need some kind of mechanism for this either way, don't we? I think
it can be a lot simpler though.
> Often being instantiated and completed in a much narrower scope, like an
executor, or in some critical path
So, literally anywhere you can have a `Future` you can have a callback with
exactly the same control flow. There's no need for extra plumbing. Wherever you
would invoke `AbstractFuture.setSuccess` or `AbstractFuture.setFailure` you can
invoke a callback.
It is true that an `Executor` lets you hand this off, but we can do that for
callbacks too. We have the power! `ExecutorPlus` can easily be given a callback
`execute` method variation, in fact this seems like a really good idea. But
also, a lambda can be submitted that simply has a try/catch branch doing the
obvious things.
> I'm not saying performance and safety are mutually exclusive, though
there's often a trade off between the two.
If I thought `Future` afforded more safety, I'd definitely agree it is a
cost worth paying, so in that respect I can absolutely see your point of view.
I just still don't see the extra safety here.
Perhaps we do as a project need to offer as much scaffolding and support for
callbacks as we already do for `Future`. Perhaps we can also consider variants
of `Future` that are more restricted but efficient (and have easier call
stacks!), so we can begin walking the two extremes closer towards each other as
well.
--
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]
