Jason Brown commented on CASSANDRA-14194:

OK, think I'm at the bottom of this. While trunk is the only branch that is 
flat-out failing the unit tests due to a problem (see below), 3.0 and 3.11 
utests are performing about 5-10% slower (when using batch commit log mode), 
but that 5-10% isn't enough to cause the test to timeout.
|[utests & 

This patch contains a set of small, subtle changes (the same across all three 

1) The main fix - There is a race in {{AbstractCommitLogService}} where the 
{{SyncRunnable}} thread where once we determine to call sync to disk (not just 
update the headers), we set {{syncRequested}} to false *after* calling 
{{commitLog.sync(true)}}. This allows us to have a race where 
{{commitLog.sync(true)}} begins it's marking work, but another mutation comes 
in, calls {{#requestExtraSync}} (which sets {{syncRequested}} to true, and 
calls unpark), and blocks in {{Allocation#awaitDiskSync()}}. If no other 
mutations come in (like in a unit test), {{AbstractCommitLogService}} will not 
{{commitLog.sync(true)}} (and {{CommitLogSegment#syncComplete}} will not 
release the blocked mutation) until the syncIntervalNanos is met - which, due 
to the introduction of the GroupCommitLog, is now hardcoded at 1000ms. By 
moving the {{syncRequested = false}} *before* the call to 
{{commitLog.sync(true)}} we eliminate the race.

2) related to #1, right before we choose to park the thread in 
{{SyncRunnable}}, we can check if {{syncRequested}} is true, and if so, avoid 
parking. This is a race between the sync thread completing the sync, but before 
it sleeps a new mutation calls {{#requestExtraSync}} (which unparks), and that 
mutation then gets stuck when the sync thread does park. If another mutation 
comes along it will unpark the sync thread, then both mutations can proceed; 
otherwise, the first mutation is blocked. This should only affect things like 
unit tests and *very* underutilized clusters. Note: I believe we've had this 
race since CASSANDRA-10202, but I'm not completely sure.

3) Fix a correctness problem, which, fortunately, has no practical implications 
- At the end of {{CommitLogSegment#sync()}} we always call 
{{syncComplete.signalAll()}}. {{syncComplete.signalAll()}} should only be 
called when we actually msync. Thus, if {{#sync()}} is called with {{flush}} = 
false, this could be improperly signalling any waiting threads. Luckily, 
{{AbstractCommitLogService}} should never be calling {{commitLog#sync(false)}} 
when in batch mode, so there is no improper signalling. However, for soundness 
of code, I'm moving the {{syncComplete.signalAll()}} call into the {{if(flush 
|| close)}} block in {{CommitLogSegment#sync()}}.

4) {{AbstractCommitLogService#thread}} is now marked volatile. This field is 
not defined until the {{start()}} method (not the constructor), so we're not 
guaranteed visibility when the {{#requestExtraSync()}} and 
{{#awaitTermination()}} methods run. (NOTE: this field was final and defined in 
the ctor until CASSANDRA-8308, introduced c* 2.2) Admittedly, I'm not thrilled 
with putting yet another volatile on the commit log path (only in batch mode, 
in {{#requestExtraSync()}}), but ... correctness? Also, reading a volatile 
variable isn't much more expensive vs. a regulat LOAD instruction. wdyt? This 
one arguably could be backported to 2.2+, but that ticket was committed 3.5 
years ago we have haven't heard anything, so maybe just 3.0+?

> Chain commit log marker potential performance regression in batch commit mode
> -----------------------------------------------------------------------------
>                 Key: CASSANDRA-14194
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-14194
>             Project: Cassandra
>          Issue Type: Bug
>          Components: Core, Testing
>            Reporter: Ariel Weisberg
>            Assignee: Jason Brown
>            Priority: Major
>         Attachments: jstack.txt
> CASSANDRA-13987 modified how the commit log sync thread works. I noticed that 
> cql3.ViewTest and ViewBuilderTaskTest have been timing out, but only in 
> CircleCI. When I jstack in CircleCI what I see is that the commit log writer 
> thread is parked and the threads trying to append to the commit log are 
> blocked waiting on it.
> I tested the commit before 13987 and it passed in CircleCI and then I tested 
> with 13987 and it timed out. I suspect this may be a general performance 
> regression and not just a CircleCI issue.
> And this is with write barriers disabled (sync thread doesn't actually sync) 
> so it wasn't blocked in the filesystem.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to