[
https://issues.apache.org/jira/browse/CASSANDRA-17103?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17602035#comment-17602035
]
Blake Eggleston commented on CASSANDRA-17103:
---------------------------------------------
[~dcapwell] I've pushed up some commits addressing your feedback.
{quote}
accord-core/src/main/java/accord/local/CommandStores.java
"forEach" takes both a "TxnOperation" and "Keys", but "TxnOperation" holds ref
to "Keys", so shouldn't we drop "Keys"? Looking at the usage, it seems like we
could...
{quote}
The keys in each perform different functions. The keys in the TxnOperation
indicate which CommandsForKey we need access to, this let's C* know which ones
it needs to load. The Keys passed into forEach determine which command stores
we need to talk to, since we need to talk to "replicas" of the given keys,
whether we need the commands for key to be in memory or not. Of course, this
isn't obvious without more complete comments on TxnOperation
{quote}
This change trickled throughout a lot of different places, so would be good to
grasp the expected semantics... if the TxnOperation has keys, and you provide
keys... "should" they be released or "must" they be related? or something else?
{quote}
I added some comments. The name TxnOperation is a little unclear. Let me know
if you have any suggestions for better names. Maybe TxnOperationScope? Maybe it
could extend {{Function<CommandStore, T>}} and bundle the operation and the
metadata into the same interface? Calls would be a bit more cumbersome, but
would make the interdependency of the 3 parts more obvious
{quote}
accord-core/src/main/java/accord/coordinate/Execute.java
We are not good with nullability in Accord, so its not clear if read is allowed
to be null... Based off Caleb's branch
org.apache.cassandra.cql3.statements.TransactionStatement#createRead won't be
null in a write only txn, but is null allowed or not? If so then we need a null
check at "if (txn.read().keys().isEmpty())"
{quote}
I've added some annotations based on how things are implemented now
{quote}
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java
I still need to look closer at our epoch logic but if we have multiple epochs
that are not full acknowledged then wouldn't forEpochCommands have the wrong
min epoch? This is only used in tests and not in the C* patch... if this is
only for tests then I am cool with this, but it would be good to start
annotating what methods are not meant to be used in `src/main`
{quote}
No I don't think so, these 2 commands just return witnessed/committed commands
belonging to the given epoch. These (and all InMemory* implementations) are
only meant to be used in tests. These methods in particular are placeholders to
range movements in the library can be validated and should be superseded by the
work to "officially" support bootstrap, low bounds, etc.
{quote}
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java
Nit: forCommittedInEpoch and forEpochCommands are mostly copy/paste of each
other, can we create a new util function such as...
{quote}
I'd tried that, but you also need to pass in the min and max timestamps, which
makes 3 arguments so you need a special interface, and the duplicated code
isn't too complex, so I figured it was a net negative for readability in
exchange for a few less lines of code
{quote}
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java
SingleThread should expose ability to control thread name... in C* we have a
"global prefix" for jvm-dtest
{quote}
This is just for testing, I don't think we need anything more sophisticated
here unless it's serving an immediate purpose.
{quote}
accord-core/src/main/java/accord/impl/InMemoryCommandStores.java
It would be nice to move "forEachLocal" to lower level... seems that we try to
overload fold in cases where forEach would be simpler.
{quote}
these are only there for testing and assume in-memory implementation details
(since none of them require a TxnScope)
{quote}
accord-core/src/main/java/accord/impl/SimpleProgressLog.java
For consistency shouldn't notPersisted be private with a getter? this patch
switches many things to getters yet makes this public field access.
{quote}
The integration of the progress log hasn't started yet. I'd expect the
interface and reference implementation to be refined as that happens, but the
objective for this patch to do the bare minimum to prevent build issues and
test failures.
{quote}
accord-core/src/main/java/accord/local/Command.java
nit: given the refactor to make all state abstract wouldn't it be best if this
became an Interface?
{quote}
I don't think so. There's a lot of functionality in there, some of which needs
to be overridden in the C* implementation
{quote}
accord-core/src/main/java/accord/local/Command.java
apply now returns a Future but some code paths ignore the output, so if apply
fails or is async, there may be unexpected results? Examples: ....
{quote}
So I don't think your example timeline is possible, since Command methods like
{{commit}} are idempotent and the command store is single threaded. T2 would be
a noop. However, I don't think there's anything to prevent {{maybeExecute}} to
fire off duplicate writes in the time between {{Command#apply}} dispatches
writes and the time {{Command#postApply}} sets the status to {{Applied}} in
{{Command#postApply}}., and the same goes for {{Command#read}}. I'm going to
think about this a bit, since it depends on the concurrency of the
implementation I'm not sure if it would be better to handle this in the
library, or in the implementation....
{quote}
accord-core/src/main/java/accord/messages/Apply.java
nit: "waitAndReduce" throws on future error rather than return a failed future,
this feels off to me but looks to be because "mapReduceLocal" doesn't allow
changing the return type; we want Void in this context but that API doesn't
allow... would it be better to allow a different return type? Caller never
checks return so Future<?> is brittle...
{quote}
Good point. Changed return type to Future<Void>
For anyone following along, Benedict has addressed my concern regarding the
paxos testing harness by quickly getting the metadata persistence code working
with the paxos tester, so I'll be pushing up bug fixes as they're found.
> CEP-15 (C*): Messaging and storage engine integration
> -----------------------------------------------------
>
> Key: CASSANDRA-17103
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17103
> Project: Cassandra
> Issue Type: New Feature
> Components: Accord
> Reporter: Benedict Elliott Smith
> Assignee: Blake Eggleston
> Priority: Normal
> Fix For: 5.x
>
>
> This work encompasses implementing Accord’s storage and networking interfaces
> within Cassandra, so that messages may be sent around the cluster and
> exectuted
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
