dcapwell commented on code in PR #3108:
URL: https://github.com/apache/cassandra/pull/3108#discussion_r1495038164
##########
src/java/org/apache/cassandra/service/accord/AccordStateCache.java:
##########
@@ -141,36 +141,45 @@ private void maybeEvictSomeNodes()
while (iter.hasNext() && bytesCached > maxSizeInBytes)
{
AccordCachingState<?, ?> node = iter.next();
- checkState(node.references == 0);
-
- if (!node.canEvict())
- continue;
- /*
- * TODO (expected, efficiency):
- * can this be reworked so we're not skipping unevictable nodes
everytime we try to evict?
- */
- Status status = node.status(); // status() call completes (if
completeable)
Review Comment:
> we've only affected the LOADING state, not the SAVING state
We are talking about both.
`org.apache.cassandra.service.accord.AccordCachingState#status`
```
public Status status()
{
return complete().status();
}
State<K, V> complete()
{
return state.isCompleteable() ? state(state.complete()) : state;
}
```
`Loading` and `Saving` are the only 2 things completable, so calling this
method on either of them promotes them to the next state.
> We seem here to only be preventing a LOADING node from being evicted
before being LOADED, which is probably a perfectly sensible thing to do. But
I'm not sure how to map to your explanation.
In the `Loading` case we promote to `Loaded` with a `null` value... when we
evict `Loaded` we double check that the value in-memory matches disk (see
`org.apache.cassandra.service.accord.AccordStateCache#evict`), this check will
fail if the data is on disk as `null != data`
> I can imagine that by evicting a LOADING command we might have a handle to
the entry somewhere that is then updated and should be saved but isn't. But
that means we've got bad reference counting or something else going on as well?
If anything has a reference to this loaded command we have a bad reference
count, so a different bug. If linked and `ref == 0` then nothing should know
about this node, so it should be safe to purge.
Now, that doesn't mean we stopped the load, so that will complete and update
the `Loading` object, but since nothing holds reference anymore that gets
garbage collected away.
--
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]
