CASSANALYTICS-124: Commitlog reading not progressing in CDC due to incorrect CommitLogReader.isFullyRead#176
Conversation
jyothsnakonisa
force-pushed
the
buffering-commit-log-reader
branch
from
2f3ecc2 to
a6a85f5
Compare
…correct CommitLogReader.isFullyRead
jyothsnakonisa
force-pushed
the
buffering-commit-log-reader
branch
from
a6a85f5 to
59b8920
Compare
| } | ||
|
|
||
| @Test | ||
| public void testPositionAtMaxOffsetWhenSeekingToEnd() |
There was a problem hiding this comment.
Excepting the assertThat(result.updates()).isEmpty(); check, this test is identical to testPositionAtMaxOffsetInCommitlogsWithPaddedZeros(). Maybe we keep this one and just leave a javadoc breadcrumb to the JIRA ticket here and the 2 cases this test covers?
| { | ||
| stats.commitLogBytesSkippedOnRead(startMarker.position() - reader.getFilePointer()); | ||
| segmentReader.seek(startMarker.position()); | ||
| // When starting from an offset, position must be initialized to startMarker.position() |
There was a problem hiding this comment.
Just leaving a note so we're aligned (no change requested). The relationship between reader, startMarker, and this.position is something we should consider structurally relating in the future. If there's an expectation that these numeric sentinels are linked, wrapping them in code that enforces those invariants would help prevent gaps and allow us to refactor more safely.
Again - nothing to do here. ;) Just my first thoughts on looking at this code in detail. I think this is worth considering for a broader refactor; this BufferingCommitLogReader seems like a leaky abstraction on top of the regular commit log reader that's forcing us to do a lot of heavy lifting and exposing us to a lot of potential pain if we change it too much. 😬
There was a problem hiding this comment.
I agree, we can note this down and do the refactoring in another patch later.
| } | ||
|
|
||
| // If the segment reader iterator completes reading commitlog with padded zeros, set the position | ||
| // as maxOffset to mark completion of reading commitlog |
There was a problem hiding this comment.
This comment confused me a bit and sent me down a rabbit hole. wdyt about this instead? Is it accurate?
// If the loop finished naturally (iterator exhausted) without hitting an error or limit,
// ensure the position reflects the end of the file. If we aborted early due to an error
// or mutation limit, 'this.position' remains at the last valid read offset.
| if (serializedSize == LEGACY_END_OF_SEGMENT_MARKER) | ||
| { | ||
| logger.trace("Encountered end of segment marker at", "position", reader.getFilePointer()); | ||
| this.position = (int) log.maxOffset(); |
There was a problem hiding this comment.
Do we have a unit test that exercises this case of "hit LEGACY_END_OF_SEGMENT_MARKER?
2 participants
No description provided.