josh-mckenzie commented on code in PR #1770:
URL: https://github.com/apache/cassandra/pull/1770#discussion_r939044799
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManagerCDC.java:
##########
@@ -190,14 +189,20 @@ public CommitLogSegment.Allocation allocate(Mutation
mutation, int size) throws
return alloc;
}
- // Non-blocking mode has just recently been enabled for CDC.
- // The segment is still marked as FORBIDDEN. It should be set to PERMITTED.
- private void ensureSegmentPermittedIfNotBlockWrites(CommitLogSegment
segment)
+ // Permit a forbidden segment under the following conditions.
+ // - Non-blocking mode has just recently been enabled for CDC.
+ // - The CDC total space has droppped below the limit (e.g. CDC consumer
cleans up).
+ private void permitSegmentMaybe(CommitLogSegment segment)
{
- if (!DatabaseDescriptor.getCDCBlockWrites() && segment.getCDCState()
== CDCState.FORBIDDEN)
+ if (segment.getCDCState() != CDCState.FORBIDDEN)
+ return;
+
+ if (!DatabaseDescriptor.getCDCBlockWrites()
+ || cdcSizeTracker.sizeInProgress.get() <
DatabaseDescriptor.getCDCTotalSpace())
Review Comment:
We _may_ want to compare to DBD.getCDCTotalSpace - the size of a CL Segment.
A hypothetical example of an edge case w/this logic here:
1. CommitLogSegment size 128mb
2. getCDCTotalSpace = 3.95G out of 4.0G
3. we see that getCDCTotalSpace is < max, so we flip the bit
4. Then we overflow by `CommitLogSegment size - (4.0 - 3.95)`
OR - we just leave it as is because this is likely not a big problem and
maybe document it. 🤷
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManagerCDC.java:
##########
@@ -386,9 +388,6 @@ private void recalculateOverflowSize()
{
rateLimiter.acquire();
calculateSize();
Review Comment:
With this change we should probably go ahead and lift up the calculateSize()
code body into the `submitOverflowSizeRecalculation` since it's only called in
one place. Saves future maintainers the jump to trace operations.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManagerCDC.java:
##########
@@ -219,6 +224,8 @@ private void throwIfForbidden(Mutation mutation,
CommitLogSegment segment) throw
/**
* On segment creation, flag whether the segment should accept CDC
mutations or not based on the total currently
* allocated unflushed CDC segments and the contents of cdc_raw
+ *
+ * Synchronized on this
Review Comment:
This is only called in one place; recommend we forklift this code out and
above to where it's called so it's clear the lock is held while these
operations take place.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentManagerCDC.java:
##########
@@ -190,14 +189,20 @@ public CommitLogSegment.Allocation allocate(Mutation
mutation, int size) throws
return alloc;
}
- // Non-blocking mode has just recently been enabled for CDC.
- // The segment is still marked as FORBIDDEN. It should be set to PERMITTED.
- private void ensureSegmentPermittedIfNotBlockWrites(CommitLogSegment
segment)
+ // Permit a forbidden segment under the following conditions.
+ // - Non-blocking mode has just recently been enabled for CDC.
+ // - The CDC total space has droppped below the limit (e.g. CDC consumer
cleans up).
+ private void permitSegmentMaybe(CommitLogSegment segment)
{
- if (!DatabaseDescriptor.getCDCBlockWrites() && segment.getCDCState()
== CDCState.FORBIDDEN)
+ if (segment.getCDCState() != CDCState.FORBIDDEN)
+ return;
+
+ if (!DatabaseDescriptor.getCDCBlockWrites()
+ || cdcSizeTracker.sizeInProgress.get() <
DatabaseDescriptor.getCDCTotalSpace())
{
segment.setCDCState(CDCState.PERMITTED);
}
+
Review Comment:
nit: extra whitespace here
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogSegmentManagerCDCTest.java:
##########
@@ -91,7 +91,7 @@ public void testCDCWriteFailure() throws Throwable
FileUtils.deleteWithConfirm(f);
// Update size tracker to reflect deleted files. Should flip flag
on current allocatingFrom to allow.
- cdcMgr.updateCDCTotalSize();
+ long x = cdcMgr.updateCDCTotalSize();
Review Comment:
unused variable declaration
--
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]
