[
https://issues.apache.org/jira/browse/CASSANDRA-21665?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Johnny Miller updated CASSANDRA-21665:
--------------------------------------
Description:
While doing some research into the commit log code I came across something that
looks like it was dropped by mistake during a merge.
CASSANDRA-18948 fixed flakiness in CommitLogSegmentManagerCDCTest by making
CommitLogSegment.cdcState volatile. That fix is present on cassandra-4.0 and
cassandra-4.1, and the 4.0 to 4.1 merge (6cac24f581) carried it correctly.
The next merge up, f48de8343c "Merge branch 'cassandra-4.1' into cassandra-5.0"
on 2023-11-20, has the volatile on its 4.1 parent but resolved
CommitLogSegment.java without it.
As a result every 5.0.x release ships "private CDCState cdcState =
CDCState.PERMITTED;" (CommitLogSegment.java, line 70 at current 5.0 HEAD) while
4.0, 4.1, 6.0 and trunk all have the volatile.
This matters because getCDCState() is read without holding cdcStateLock on the
CDC allocation hot path (CommitLogSegmentManagerCDC.permitSegmentMaybe and
throwIfForbidden, lines 197 and 214 at 5.0 HEAD, and createSegment at line 243)
and in CommitLogSegment.sync (line 363), so 5.0 has the cross-thread visibility
gap CASSANDRA-18948 fixed, along with the CDC test flakiness it was addressing.
I think this would result in threads sometimes seeing an out of date CDC state
for a segment. So a CDC write could be rejected when there is actually space,
or let through when there is not, and the _cdc.idx file could be written one
sync later than it should. Nothing gets lost, it just makes the wrong call for
a moment, and it brings back the flaky CDC tests that CASSANDRA-18948 was
fixing. Happy to be told it is fine without it, but every other branch has it,
so it looks like a merge accident.
The fix is the original one line, applied to cassandra-5.0 only; cassandra-6.0
and trunk already carry the volatile.
Patch to follow.
was:
While doing some research into the commit log code I came across something that
looks like it was dropped by mistake during a merge.
CASSANDRA-18948 fixed flakiness in CommitLogSegmentManagerCDCTest by making
CommitLogSegment.cdcState volatile. That fix is present on cassandra-4.0 and
cassandra-4.1, and the 4.0 to 4.1 merge (6cac24f581) carried it correctly.
The next merge up, f48de8343c "Merge branch 'cassandra-4.1' into cassandra-5.0"
on 2023-11-20, has the volatile on its 4.1 parent but resolved
CommitLogSegment.java without it.
As a result every 5.0.x release ships "private CDCState cdcState =
CDCState.PERMITTED;" (CommitLogSegment.java, line 70 at current 5.0 HEAD) while
4.0, 4.1, 6.0 and trunk all have the volatile.
This matters because getCDCState() is read without holding cdcStateLock on the
CDC allocation hot path (CommitLogSegmentManagerCDC.permitSegmentMaybe and
throwIfForbidden, lines 197 and 214 at 5.0 HEAD, and createSegment at line 243)
and in CommitLogSegment.sync (line 363), so 5.0 has the cross-thread visibility
gap CASSANDRA-18948 fixed, along with the CDC test flakiness it was addressing.
I think this would result in threads sometimes seeing an out of date CDC state
for a segment. So a CDC write could be rejected when there is actually space,
or let through when there is not, and the _cdc.idx file could be written one
sync later than it should. Nothing gets lost, it just makes the wrong call for
a moment, and it brings back the flaky CDC tests that CASSANDRA-18948 was
fixing. Happy to be told it is fine without it, but every other branch has it,
so it looks like a merge accident.
The fix is the original one line, applied to cassandra-5.0 only; cassandra-6.0
and trunk already carry the volatile, so the merge upwards is a no-op.
Patch to follow.
> Restore volatile on CommitLogSegment.cdcState, lost in the cassandra-4.1 to
> cassandra-5.0 merge
> -----------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-21665
> URL: https://issues.apache.org/jira/browse/CASSANDRA-21665
> Project: Apache Cassandra
> Issue Type: Bug
> Components: Local/Commit Log
> Reporter: Johnny Miller
> Assignee: Johnny Miller
> Priority: Normal
> Fix For: 5.0.x
>
>
> While doing some research into the commit log code I came across something
> that looks like it was dropped by mistake during a merge.
> CASSANDRA-18948 fixed flakiness in CommitLogSegmentManagerCDCTest by making
> CommitLogSegment.cdcState volatile. That fix is present on cassandra-4.0 and
> cassandra-4.1, and the 4.0 to 4.1 merge (6cac24f581) carried it correctly.
> The next merge up, f48de8343c "Merge branch 'cassandra-4.1' into
> cassandra-5.0" on 2023-11-20, has the volatile on its 4.1 parent but resolved
> CommitLogSegment.java without it.
> As a result every 5.0.x release ships "private CDCState cdcState =
> CDCState.PERMITTED;" (CommitLogSegment.java, line 70 at current 5.0 HEAD)
> while 4.0, 4.1, 6.0 and trunk all have the volatile.
> This matters because getCDCState() is read without holding cdcStateLock on
> the CDC allocation hot path (CommitLogSegmentManagerCDC.permitSegmentMaybe
> and throwIfForbidden, lines 197 and 214 at 5.0 HEAD, and createSegment at
> line 243) and in CommitLogSegment.sync (line 363), so 5.0 has the
> cross-thread visibility gap CASSANDRA-18948 fixed, along with the CDC test
> flakiness it was addressing.
> I think this would result in threads sometimes seeing an out of date CDC
> state for a segment. So a CDC write could be rejected when there is actually
> space, or let through when there is not, and the _cdc.idx file could be
> written one sync later than it should. Nothing gets lost, it just makes the
> wrong call for a moment, and it brings back the flaky CDC tests that
> CASSANDRA-18948 was fixing. Happy to be told it is fine without it, but every
> other branch has it, so it looks like a merge accident.
> The fix is the original one line, applied to cassandra-5.0 only;
> cassandra-6.0 and trunk already carry the volatile.
> Patch to follow.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
