CEP-15 Accord: Immutable state integration #2144
Conversation
patch by Blake Eggleston; reviewed by Benedict Elliott Smith, David Capwell for CASSANDRA-17103
…an rely on a version column, and fixed a few feedback related issues patch by David Capwell; reviewed by Blake Eggleston for CASSANDRA-17103
The replication of transactions and their processing state must be optimal, i.e. linearly proportional to the size of the transaction and the number of keys involved. This patch ensures a transaction is replicated in slices, with each slice being written (replication factor) times only to the shard that owns the keys, and only once (except in the case the transaction is recovered). patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-17109
Ensure old or abandoned transactions are invalidated wherever possible. patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-18041
…ce dedicated property tests; re-activate fast-path during range movements patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-18056
- Integrate accord-core changes for CASSANDRA-18057 patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-18057
- Integrate accord-core changes for CASSANDRA-18087 patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-18087
patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-17719 Co-authored-by: Caleb Rackliffe <calebrackliffe@gmail.com> Co-authored-by: Blake Eggleston <bdeggleston@gmail.com> Co-authored-by: David Capwell <dcapwell@apache.org>
…ither the current or the user value are NULL
…dFuture to get simulator working
…ants.NULL_VALUE rather than try to parse it
…ion, will revert before trunk but needed for debugging
make sure to update this.kind in order to make tests stable
AsyncWriterTest#commandsPerKeyDenormalization to use SaveStatus rather than Status
…decreaseSlightly to get back to the old behavior for now
…utingKey was added but does not work in all cases in C* due to sentinal values, added logic to return a C* friendly token
Some tests may be flaky now due to Preempted being thrown.
…ng IN clauses to return multiple partitions/rows patch by David Capwell; reviewed by Caleb Rackliffe for CASSANDRA-18154
patch by Benedict; reviewed by Aleksey Yeschenko for CASSANDRA-18142
- Combine real and logical into a single 64-but HLC - Introduce 16 flag bits - Pack epoch (48-bits), HLC (64-bits) and flags (16-bits) into two longs in memory patch by Benedict; reviewed by Aleksey Yeschenko for CASSANDRA-18172
patch by Ariel Weisberg; Reviewed by Caleb Rackliffe for CASSANDRA-18100
…and clustering IN clauses to return multiple partitions/rows
Refactor Deps into KeyDeps and RangeDeps patch by Benedict; reviewed by Aleksey Yeschenko for CASSANDRA-18173
patch by Benedict; reviewed by Ariel Weisberg for CASSANDRA-18174
patch by Caleb Rackliffe; reviewed by David Capwell for CASSANDRA-18195
src/java/org/apache/cassandra/service/accord/async/AsyncLoader.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/service/accord/async/AsyncWriter.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/service/accord/serializers/CommandsForKeySerializer.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/service/accord/async/AsyncOperation.java
Outdated
Show resolved
Hide resolved
patch by <jacek-lewandowski>; reviewed by <maedhroz> and <dcapwell> for CASSANDRA-18241
patch by David Capwell; reviewed by Caleb Rackliffe, Michael Semb Wever for CASSANDRA-18204
|
Ran stress and here are exceptions that I saw |
bdeggleston
force-pushed
the
immutable-state
branch
from
494c14d to
a1d56b9
Compare
…ative paths during development patch by David Capwell; reviewed by Caleb Rackliffe, Michael Semb Wever for CASSANDRA-18204
patch by Jacek Lewandowski; reviewed by Caleb Rackliffe for CASSANDRA-18240
dcapwell
left a comment
dcapwell
left a comment
There was a problem hiding this comment.
small changes, but LGTM +1
src/java/org/apache/cassandra/service/accord/AccordSafeCommandsForKey.java
Show resolved
Hide resolved
| default void checkNotInvalidated() | ||
| { | ||
| if (invalidated()) | ||
| throw new IllegalStateException("Cannot access invalidated " + this); |
There was a problem hiding this comment.
fixing the toString we get
ERROR [CommandStore[12]:1] 2023-03-02 18:00:33,137 AsyncOperation.java:274 - Operation AsyncOperation{SAVING}-0x7bd3b9c2 failed
java.lang.IllegalStateException: Cannot access invalidated AccordSafeCommand{invalidated=true, global=Node{LOADED, key=[1,1677808833023004,2,2130706433], references=1}@2de5a0d2, original=null, current=Command@1167976198{[1,1677808833023004,2,213
0706433]:PreAccepted}}
at org.apache.cassandra.service.accord.AccordSafeState.checkNotInvalidated(AccordSafeState.java:74)
at org.apache.cassandra.service.accord.AccordSafeCommand.original(AccordSafeCommand.java:94)
at org.apache.cassandra.service.accord.AccordSafeCommand.original(AccordSafeCommand.java:29)
at org.apache.cassandra.service.accord.AccordSafeState.hasUpdate(AccordSafeState.java:38)
at org.apache.cassandra.service.accord.async.AsyncWriter.lambda$assembleWrites$1(AsyncWriter.java:85)
at java.base/java.util.HashMap.forEach(HashMap.java:1337)
at org.apache.cassandra.service.accord.async.AsyncWriter.assembleWrites(AsyncWriter.java:84)
at org.apache.cassandra.service.accord.async.AsyncWriter.maybeDispatchWrites(AsyncWriter.java:116)
at org.apache.cassandra.service.accord.async.AsyncWriter.save(AsyncWriter.java:155)
at org.apache.cassandra.service.accord.async.AsyncOperation.runInternal(AsyncOperation.java:235)
at org.apache.cassandra.service.accord.async.AsyncOperation.run(AsyncOperation.java:270)
at org.apache.cassandra.service.accord.async.AsyncOperation.callback(AsyncOperation.java:158)
There was a problem hiding this comment.
java.lang.IllegalStateException: Cannot access invalidated AccordSafeCommand{invalidated=true, global=Node{LOADED, key=[1,1677809078513005,2,2130706433], references=3}@36b1ca9e, original=Command@1483650346{[1,1677809078513005,2,2130706433]:PreAc
cepted}, current=Command@1483650346{[1,1677809078513005,2,2130706433]:PreAccepted}}
at org.apache.cassandra.service.accord.AccordSafeState.checkNotInvalidated(AccordSafeState.java:74)
at org.apache.cassandra.service.accord.AccordSafeCommand.current(AccordSafeCommand.java:80)
at org.apache.cassandra.service.accord.AccordSafeCommand.current(AccordSafeCommand.java:29)
at org.apache.cassandra.service.accord.AccordSafeState.hasUpdate(AccordSafeState.java:38)
at org.apache.cassandra.service.accord.async.AsyncWriter.lambda$assembleWrites$1(AsyncWriter.java:85)
at java.base/java.util.HashMap.forEach(HashMap.java:1337)
at org.apache.cassandra.service.accord.async.AsyncWriter.assembleWrites(AsyncWriter.java:84)
at org.apache.cassandra.service.accord.async.AsyncWriter.maybeDispatchWrites(AsyncWriter.java:116)
at org.apache.cassandra.service.accord.async.AsyncWriter.save(AsyncWriter.java:155)
at org.apache.cassandra.service.accord.async.AsyncOperation.runInternal(AsyncOperation.java:235)
at org.apache.cassandra.service.accord.async.AsyncOperation.run(AsyncOperation.java:270)```
There was a problem hiding this comment.
org.apache.cassandra.service.accord.async.AsyncOperation#runInternal calls
commandStore.completeOperation(safeStore, context.commands, context.commandsForKeys);
which makes it unsafe for writer to have access to org.apache.cassandra.service.accord.async.AsyncOperation.Context#commands
So, for this to work we need to do completeOperation after the write, or make the writer context not have access; or have the read methods supported... which kinda invalidates... this logic?
dcapwell
left a comment
dcapwell
left a comment
There was a problem hiding this comment.
removing +1 due to issues found while running bin/cassandra -f
|
Errors seen while trying to run with load |
this next one is a known one |
|
updated SPL to show details when it fails, this is what I got for code |
src/java/org/apache/cassandra/service/accord/async/AsyncLoader.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/service/accord/AccordStateCache.java
Outdated
Show resolved
Hide resolved
| this.commandStore = commandStore; | ||
| this.txnIds = txnIds; | ||
| this.keys = keys; | ||
| this.txnIds = Lists.newArrayList(txnIds); |
There was a problem hiding this comment.
Why do we copy these to a new List? If we need a List (or a Set somewhere else) we should try to specify this in PreLoadContext imo.
| this.txnIds = txnIds; | ||
| this.keys = keys; | ||
| this.txnIds = Lists.newArrayList(txnIds); | ||
| this.keys = Lists.newArrayList(keys); |
There was a problem hiding this comment.
Should we just accept an AbstractKeys<RoutingKey, ?> instead?
| private final NamedMap<Object, Future<?>> writeFutures = new NamedMap<>("writeFutures"); | ||
| private final NamedMap<Object, AsyncResult<Void>> saveResults = new NamedMap<>("saveResults"); | ||
|
|
||
| private int linked = 0; |
There was a problem hiding this comment.
that's misleading, they're not neccesarily eligibleForEviction. unreferenced is probably clearer than linked though
There was a problem hiding this comment.
Doesn’t unreferenced basically mean eligible for eviction (if last in queue)? Except for the condition that it’s loaded (that we can discuss in another thread)
either way, unreferenced is def an improvement
There was a problem hiding this comment.
Almost. To be eligible for eviction, there also needs to be no unfinished operations, it needs to be in the right state, etc.
There was a problem hiding this comment.
Should we even be inserting into the list if we aren’t yet eligible for collection? Doesn’t that cause inefficiencies when actually evicting?
There was a problem hiding this comment.
Once a minute is definitely not enough. You do not need much traffic to exhaust the cache in that time interval. Let's say you have 128MiB cache, 5k qps, you only 450 bytes in cache per query to exhaust the cache. Just 50 bytes per query wastes 10% of your cache space.
I'm strongly -1 on timed cache clearing. Cache management should dynamic based on actual demand and conditions. If we decide to accept the pointer chasing cost that's one thing, but a cache clearance frequency is another tunable we shouldn't have, and we have enough of those.
I don't mind punting on this, with a TODO (expected) to revisit, perhaps when we address persistence. But not with a periodic clearance, let's keep doing this on demand.
There was a problem hiding this comment.
(There remain some other very simple possibilities, such as dequeuing the tail elements that we want to evict but can't, and place them in a separate queue for eviction as soon as they are ready, so that we don't repeatedly walk over them.)
There was a problem hiding this comment.
Ok one minute was a bit of an exaggeration. I am in favor of accepting the pointer chasing cost until we can demonstrate it's a bottleneck though.
There was a problem hiding this comment.
I'm happy to punt it as explicit technical debt, but don’t want to waive it away with a "prove it’s a problem" mentality. That shouldn’t be how we address concerns like this.
I think we probably agree that we would prefer our design not to have this property? and that we’re mostly willing to accept it because we have bigger fish to fry. In which case it’s probably a worse investment of our time to hash out the relevant standard of proof than it would be to address the issue, should it come to it.
But I agree it's not an optimal use of our time right now.
There was a problem hiding this comment.
Not trying to wave it away, or get into any Mexican standoffs :)
| } | ||
|
|
||
| boolean isEmpty() | ||
| private boolean isUnlinked() |
There was a problem hiding this comment.
isEligibleForEviction?
There was a problem hiding this comment.
I'd prefer to keep as is. This is literally checking that the prev/next fields are null, and is used as a sanity check before updating the linked list
There was a problem hiding this comment.
Unlinked felt ambiguous to me as a reader - the next and prev are potentially links, but so potentially are the references. Perhaps isInEvictionQueue?
src/java/org/apache/cassandra/service/accord/AccordStateCache.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/service/accord/AccordStateCache.java
Outdated
Show resolved
Hide resolved
| enum State | ||
| { | ||
| INITIALIZED, | ||
| SUBMITTED, |
There was a problem hiding this comment.
Unused? Would be good in general to comment these, at least the not 100% obvious ones like difference between SAVING and AWAITING_SAVE, LOADING and PREPARING.
| */ | ||
| public class AccordLoadingState<K, V> | ||
| { | ||
| public enum LoadingState { NOT_FOUND, PENDING, LOADED, FAILED } |
There was a problem hiding this comment.
NOT_FOUND -> UNINITIALIZED?
I spent a while this morning thinking this meant it wasn't found on disk, and trying to figure out how this differed from LOADED.
| private final NamedMap<Object, AsyncResult<Void>> saveResults = new NamedMap<>("saveResults"); | ||
|
|
||
| private int unreferenced = 0; | ||
| Node<?, ?> head; |
There was a problem hiding this comment.
We have a doubly-linked list, and idiomatically these have a dummy header (embedded in the owning class, i.e. so that AccordStateCache would extend Node or some super class). We have templates for this already around the codebase, e.g. IntrusiveLinkedList.
I'd be happy to convert if you've no concerns, or there's a reason I'm missing.
There was a problem hiding this comment.
Node extends AccordLoadingState so it can't also extend some base node class. We also don't want the state cache inheriting the accord loading state stuff.
There was a problem hiding this comment.
We can invert that hierarchy quite easily can't we? The only class extending it is Node, so if we are doing it to separate concerns why not just have a LoadingNode extends Node or something?
There was a problem hiding this comment.
Node also has a bunch of stuff we don’t want on the cache, and I don’t like the idea of AccordLoadingState extending a Node type. It could be done with interfaces, albeit more awkwardly, but I don’t really see the benefit, aside from slightly simpler push/pop methods?
There was a problem hiding this comment.
Why don't you like inverting the inheritance order?
It's not just that we get simpler push/pop methods: we can reuse the existing intrusive linked lists, so we are duplicating less functionality. It's also more idiomatic, so it's less surprising to a reader familiar with such lists.
There was a problem hiding this comment.
If you don't want to swap the inheritance order of course, we don't need to have AccordStateCache extend Node, we can just have a dummy member field. It's just very marginally less efficient/neat, and we also avoid the aforementioned code-reuse benefits.
So, I still think it would be better to inherit since we can. But we can still gain the benefits of a circular-linked list without doing so,
There was a problem hiding this comment.
Why don't you like inverting the inheritance order?
Because Node is an implementation detail of the cache, and not the logical ancestor of accord state objects.
It's also more idiomatic, so it's less surprising to a reader familiar with such lists.
I’d disagree with that. Having explicit head/tail pointers on the list container is much more idiomatic than a container also being a dummy node. You could argue that this does put head/tail fields on the container, but it’s not nearly as straightforward or easy to identify. It does enable cleaner list manipulation methods, but is not the least surprising approach.
It also breaks the idiomatic linked list traversal pattern, and creates a special node you need to be aware of.
There was a problem hiding this comment.
🤷 I guess this is kind of like a blue/white dress thing. I thought it was pretty widely accepted that sentinel nodes and circular lists were the most common way of implementing doubly-linked lists. Certainly not singly linked lists, in which I would expect separate start/end pointers. But, no point arguing if we don't have the same ground truths there, we will get nowhere.
Because Node is an implementation detail of the cache, and not the logical ancestor of accord state objects.
I'm confused. The AccordLoadingState is only extended by Node, and is only used to expose the inner enum LoadingState AFAICT outside of the cache hierarchy. What accord state objects would we be the logical ancestor of?
There was a problem hiding this comment.
I'm confused. The AccordLoadingState is only extended by Node, and is only used to expose the inner enum LoadingState AFAICT outside of the cache hierarchy. What accord state objects would we be the logical ancestor of?
Well none*, it's a conceptual and structural thing. It keeps things neater and more understandable if they can be kept separate
- a test class does extend it, but it's not really consequential.
| { | ||
| F r = futuresMap.get(key); | ||
| logger.trace("Evicting {} {} - {}", evict.state(), evict.key(), evict.isLoaded() ? evict.value() : null); | ||
| if (unlink) unlink(evict); |
There was a problem hiding this comment.
Might it be neater to just have unlink no-op if it's not in the queue?
There was a problem hiding this comment.
I wrote this to maintain assumption checks, if we believe it should be linked but it isn't, this is a bug we should fail on.
There was a problem hiding this comment.
Isn't this more brittle, since we could be linked and think we aren't, and do nothing?
There was a problem hiding this comment.
Blake added a patch to verify this assumption in the else block.
since we could be linked and think we aren't,
This would be a bug, and not detecting that could cause a use-after-free issue (which leads to a NPE later on). There was a similar issue earlier on where the node was not in cache but it was in the linked list, this eventually NPE.
patch by David Capwell; reviewed by Alex Petrov, Caleb Rackliffe, Jacek Lewandowski for CASSANDRA-18299
| node.isLoaded() && | ||
| !hasActiveAsyncResult(saveResults, node.key()); | ||
| Invariants.checkState(node.references == 0); | ||
| return node.state() == FAILED || !hasActiveAsyncResult(saveResults, node.key()); |
There was a problem hiding this comment.
this change wasn't replicated to org.apache.cassandra.service.accord.AccordStateCache.Instance#checkCanEvict. This is only used in org.apache.cassandra.service.accord.async.AsyncOperationTest#assertCanEvict and was there to have a more human readable understanding of why we think we can not evict...
I am ok dropping org.apache.cassandra.service.accord.AccordStateCache.Instance#checkCanEvict as its just for tests and causes duplication of logic; but does make bugs harder to track down when tests do finally fail...
|
|
||
| private static RuntimeException throwPreempted(TxnId txnId, Txn txn, ConsistencyLevel consistencyLevel) | ||
| { | ||
| throw txn.isWrite() ? new WritePreemptedException(WriteType.TRANSACTION, consistencyLevel, 0, 0, txnId.toString()) |
There was a problem hiding this comment.
when you rebase this will become a regression, can you use CAS? I document why in CASSANDRA-18299; TRANSACTION requires driver/protocol changes
|
I am mostly cool with all the changes since last approval. I still have 2 concerns but think it is ok to merge while we try to figure out what happened; while running perf tests I hit 2 issues: many transactions depended on a PreAccepted one that never made progress, and 2 transactions depended on NotWitnessed transactions (150 and 260) but this was a single node test only I know burn tests have been running and no repo there yet, and once this patch rebases to pick up CASSANDRA-18299 I can rerun benchmarks to see if I can repo again... |
Patch by Blake Eggleston; Reviewed by David Capwell & Benedict Elliott Smith for CASSANDRA-18004
Patch by Blake Eggleston; Reviewed by David Capwell and Benedict Elliott Smith for Cassandra-18192
bdeggleston
force-pushed
the
immutable-state
branch
from
73af3eb to
6c220c0
Compare
7 participants
No description provided.