[
https://issues.apache.org/jira/browse/CASSANDRA-17519?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17543984#comment-17543984
]
Benedict Elliott Smith commented on CASSANDRA-17519:
----------------------------------------------------
Hi Jakub,
Thanks for the patch.
So, the 4.1 code I think is essentially correct already if the rest of the
system is correct. If a new ref is taken to an _obsoleted_ sstable that has had
its global reference released and all of its other extant references, then
we're already in bug territory as the file is likely to disappear in some of
these cases before we can use it. Whereas if there is no obsoletion to run, it
is fine to have multiple tidies.
As far as I am aware, in a steady-state system there is no way to encounter
this situation without a bug. I believe there is some startup behaviour that
_can_ trigger this behaviour, but I am uncertain if it can trigger it with
obsoletions involved. Probably we should create some additional debugging to
ensure these constraints are held and provide information to track down
locations that break them.
Still, it can be improved. In particular we can erase a source of these race
conditions by cancelling new `Ref` we create and then discard, so that we do
not run their tidy. We can also avoid having multiple tidies in flight, and
even behave better in the case where we detect such a bug and have the chance
to stop the damage.
Anyway, I have a slightly modified
[patch|https://github.com/belliottsmith/cassandra/tree/17519-4.1-suggest]
proposal to avoid yielding/spinning. This approach is non-blocking as it
guarantees forward progress. Essentially we cancel the prior tidy by removing
it from the map if it had not yet completed, but adopt any obsoletion it would
have run, and log an exception if this ever occurs (as it really should not).
Let me know what you think.
> races/leaks in SSTableReader::GlobalTidy
> ----------------------------------------
>
> Key: CASSANDRA-17519
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17519
> Project: Cassandra
> Issue Type: Bug
> Components: Legacy/Core
> Reporter: Jakub Zytka
> Assignee: Jakub Zytka
> Priority: Normal
> Attachments: CASSANDRA-17519-4.0.txt, CASSANDRA-17519-4.1-fix.txt,
> CASSANDRA-17519-4.1-test-exposing-the-problem.txt
>
>
> In Cassandra 4.0/3.11 there are at least two races in
> SSTableReader::GlobalTidy
> One is a get/get race, explicitly handled as an assertion in:
> [http://github.com/apache/cassandra/blob/c22accc46458d0a583afcf6a980f731cdcc94465/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java#L2199-L2204]
> and it looks like "ok, it's a problem, but let's just not fix it"
> The other one is get/tidy race between
> [http://github.com/apache/cassandra/blob/c22accc46458d0a583afcf6a980f731cdcc94465/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java#L2194-L2196]
> and
> [http://github.com/apache/cassandra/blob/c22accc46458d0a583afcf6a980f731cdcc94465/src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java#L2174-L2175]
>
> The second one can be easily hit by adding a small delay at the beginning of
> `tidy()` method (say, 20ms) and running `LongStreamingTest` (and actually
> such failure is what prompted the investigation of GlobalTidy correctness)
> There was an attempt on `trunk` to fix these two races.
> The details are not clear to me, and it all looks quite weird. I might be
> mistaken, but as far as I can see the relevant changes were introduced in:
> [https://github.com/apache/cassandra/commit/31bea0b0d41e4e81095f0d088094f03db14af490]
> that is piggybacked on a huge change in CASSANDRA-17008, without a separate
> ticket or any sort of qa.
> As far as I can see this attempt changes the first race into a leak, and the
> second race to another race, this time allowing to have multiple GlobalTidy
> objects for the same sstable (and, as a result, a premature running of
> obsoletion code).
> I'll follow up with PRs for relevant branches etc etc
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
