You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "netudima (via GitHub)" <gi...@apache.org> on 2025/07/28 11:12:54 UTC
[PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
netudima opened a new pull request, #4276:
URL: https://github.com/apache/cassandra/pull/4276
Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors
Patch by Dmitry Konstantinov; reviewed by TBD for CASSANDRA-20664
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2236487271
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java:
##########
@@ -101,56 +101,69 @@ protected SyncSegment computeNext()
{
while (true)
{
+ final int currentStart = end;
try
{
- final int currentStart = end;
end = readSyncMarker(descriptor, currentStart, reader);
- if (end == -1)
- {
- return endOfData();
- }
- if (end > reader.length())
- {
- // the CRC was good (meaning it was good when it was written and still looks legit), but the file is truncated now.
- // try to grab and use as much of the file as possible, which might be nothing if the end of the file truly is corrupt
- end = (int) reader.length();
- }
+ }
+ catch (CommitLogSegmentReader.SegmentReadException e)
+ {
+ handleUnrecoverableError(e, !e.invalidCrc && tolerateTruncation);
+ end = -1; // skip the remaining part of the corrupted log segment
Review Comment:
So just to iterate on the logic here ... when `readSyncMarker` throws `SegmentReadException` and it is evaluated as _permissible_, then `handleUnrecoverableError` will not throw, so `end` will be set to `-1`, hence we will `return endOfData();` later on.
If it is thrown and not permissible, we will throw `RuntimeException` from `handleUnrecoverableError`.
Then I see that when it is thrown like that, reader is iterated over in `CommitLogReader#readCommitLogSegment`, which is wrapped by try-catch and catches RTE and throws again ...
Yeah I think this is legit.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2236565722
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogReaderTest.java:
##########
@@ -165,6 +186,54 @@ public void testReadCountFromMidpoint() throws Throwable
confirmReadOrder(testHandler, samples / 2);
}
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsDisabled() throws Throwable
Review Comment:
this test fails when executed alone.
Also please consider the usage
````
Assertions.assertThatThrownBy(() -> reader.readCommitLogSegment(clrHandler,
corruptedSegmentFile,
CommitLogPosition.NONE,
CommitLogReader.ALL_MUTATIONS,
false))
.isInstanceOf(CommitLogReplayer.CommitLogReplayException.class);
````
instead.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2239560217
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogReaderTest.java:
##########
@@ -165,6 +182,59 @@ public void testReadCountFromMidpoint() throws Throwable
confirmReadOrder(testHandler, samples / 2);
}
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsDisabled() throws Throwable
+ {
+ DatabaseDescriptor.setCommitFailurePolicy(Config.CommitFailurePolicy.stop);
+
+ File corruptedSegmentFile = createAndWriteCorruptedCommitLogFile();
+ CommitLogReader reader = new CommitLogReader();
+ // use real CLR handler to test actual behavior
+ CommitLogReadHandler clrHandler =
+ new CommitLogReplayer(new CommitLog(null), null, null, null);
+
+ // ignore.replay.errors disabled, so we expect the exception here
+ Assertions.assertThatThrownBy(() ->
+ reader.readCommitLogSegment(clrHandler,
+ corruptedSegmentFile,
+ CommitLogPosition.NONE,
+ CommitLogReader.ALL_MUTATIONS,
+ false)
+ ).isInstanceOf(CommitLogReplayer.CommitLogReplayException.class);
+ }
+
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsEnabled() throws Throwable
+ {
+ System.setProperty(IGNORE_REPLAY_ERRORS_PROPERTY, "true");
Review Comment:
The test passes as well when this is commented out.
I would do
````
@Test
public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsEnabled() throws Throwable
{
try (WithProperties properties = new WithProperties(IGNORE_REPLAY_ERRORS_PROPERTY, "true"))
{
DatabaseDescriptor.setCommitFailurePolicy(Config.CommitFailurePolicy.stop);
...
````
and remove whole `@After`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2237343676
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java:
##########
@@ -101,56 +101,69 @@ protected SyncSegment computeNext()
{
while (true)
{
+ final int currentStart = end;
try
{
- final int currentStart = end;
end = readSyncMarker(descriptor, currentStart, reader);
- if (end == -1)
- {
- return endOfData();
- }
- if (end > reader.length())
- {
- // the CRC was good (meaning it was good when it was written and still looks legit), but the file is truncated now.
- // try to grab and use as much of the file as possible, which might be nothing if the end of the file truly is corrupt
- end = (int) reader.length();
- }
+ }
+ catch (CommitLogSegmentReader.SegmentReadException e)
+ {
+ handleUnrecoverableError(e, !e.invalidCrc && tolerateTruncation);
+ end = -1; // skip the remaining part of the corrupted log segment
Review Comment:
yes, the essential change is adding end = -1 to move to the end of the corrupted segment, previously when we ignored an error we started to read the corrupted place again from the same place, so it was an infinite loop.
handleUnrecoverableError contains the existing logic, I've just moved the repeating logic into an utility method to make the main loop logic more clear.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2236565722
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogReaderTest.java:
##########
@@ -165,6 +186,54 @@ public void testReadCountFromMidpoint() throws Throwable
confirmReadOrder(testHandler, samples / 2);
}
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsDisabled() throws Throwable
Review Comment:
this test fails when executed alone.
Also please consider the usage
````
Assertions.assertThatThrownBy(() -> reader.readCommitLogSegment(clrHandler,
corruptedSegmentFile,
CommitLogPosition.NONE,
CommitLogReader.ALL_MUTATIONS,
false))
.isInstanceOf(CommitLogReplayer.CommitLogReplayException.class);
````
instead.
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogReaderTest.java:
##########
@@ -165,6 +186,54 @@ public void testReadCountFromMidpoint() throws Throwable
confirmReadOrder(testHandler, samples / 2);
}
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsDisabled() throws Throwable
Review Comment:
this test fails when executed alone.
Also please consider the usage
````
Assertions.assertThatThrownBy(() -> reader.readCommitLogSegment(clrHandler,
corruptedSegmentFile,
CommitLogPosition.NONE,
CommitLogReader.ALL_MUTATIONS,
false))
.isInstanceOf(CommitLogReplayer.CommitLogReplayException.class);
````
instead.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2236565722
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogReaderTest.java:
##########
@@ -165,6 +186,54 @@ public void testReadCountFromMidpoint() throws Throwable
confirmReadOrder(testHandler, samples / 2);
}
+ @Test
+ public void testSyncMarkerChecksumReadFailed_ignoreReplayErrorsDisabled() throws Throwable
Review Comment:
this test fails when executed alone.
Also please consider the usage
````
Assertions.assertThatThrownBy(() -> reader.readCommitLogSegment(clrHandler,
corruptedSegmentFile,
CommitLogPosition.NONE,
CommitLogReader.ALL_MUTATIONS,
false))
.isInstanceOf(CommitLogReplayer.CommitLogReplayException.class);
````
instead.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4276:
URL: https://github.com/apache/cassandra/pull/4276#discussion_r2236487271
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java:
##########
@@ -101,56 +101,69 @@ protected SyncSegment computeNext()
{
while (true)
{
+ final int currentStart = end;
try
{
- final int currentStart = end;
end = readSyncMarker(descriptor, currentStart, reader);
- if (end == -1)
- {
- return endOfData();
- }
- if (end > reader.length())
- {
- // the CRC was good (meaning it was good when it was written and still looks legit), but the file is truncated now.
- // try to grab and use as much of the file as possible, which might be nothing if the end of the file truly is corrupt
- end = (int) reader.length();
- }
+ }
+ catch (CommitLogSegmentReader.SegmentReadException e)
+ {
+ handleUnrecoverableError(e, !e.invalidCrc && tolerateTruncation);
+ end = -1; // skip the remaining part of the corrupted log segment
Review Comment:
So just to iterate on the logic here ... when `readSyncMarker` throws `SegmentReadException` and it is evaluated as it is _permissible_, then `handleUnrecoverableError` will not throw, so `end` will be set to `-1`, hence we will `return endOfData();` later on.
If it is thrown and not permissible, we will throw `RuntimeException` from `handleUnrecoverableError`.
Then I see that when it is thrown like that, reader is iterated over in `CommitLogReader#readCommitLogSegment`, which is wrapped by try-catch and catches RTE and throws again ...
Yeah I think this is legit.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors [cassandra]
Posted by "netudima (via GitHub)" <gi...@apache.org>.
netudima closed pull request #4276: [CASSANDRA-20664][4.1] Improve CommitLogSegmentReader to skip SyncBlocks correctly in case of CRC errors
URL: https://github.com/apache/cassandra/pull/4276
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org