josh-mckenzie commented on a change in pull request #1164:
URL: https://github.com/apache/cassandra/pull/1164#discussion_r695773060
##########
File path:
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java
##########
@@ -151,8 +165,22 @@ private int readSyncMarker(CommitLogDescriptor descriptor,
int offset, RandomAcc
updateChecksumInt(crc, (int) reader.getPosition());
final int end = reader.readInt();
long filecrc = reader.readInt() & 0xffffffffL;
+
if (crc.getValue() != filecrc)
{
+ // The next marker position and CRC value are not writen
atomically, so it is possible for the latter to
+ // still be zero after the former has been finalized, even though
the mutations that follow it are valid.
+ // When there is no compression or encryption enabled, we can
ignore a sync marker CRC mismatch and defer
+ // to the per-mutation CRCs, which may be preferable to preventing
startup altogether.
+ if (allowSkipSyncMarkerCrc
+ && descriptor.compression == null &&
!descriptor.getEncryptionContext().isEnabled()
+ && filecrc == 0 && end != 0)
+ {
+ logger.warn("Skipping sync marker CRC check at position {}
(end={}, calculated crc={}) of commit log {}...",
Review comment:
Consider adding something to the log about "Using per mutation CRC
checks to ensure correctness instead"
##########
File path:
src/java/org/apache/cassandra/db/commitlog/CommitLogSegmentReader.java
##########
@@ -151,8 +165,22 @@ private int readSyncMarker(CommitLogDescriptor descriptor,
int offset, RandomAcc
updateChecksumInt(crc, (int) reader.getPosition());
final int end = reader.readInt();
long filecrc = reader.readInt() & 0xffffffffL;
+
if (crc.getValue() != filecrc)
{
+ // The next marker position and CRC value are not writen
atomically, so it is possible for the latter to
Review comment:
nit: written
##########
File path: test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java
##########
@@ -764,6 +782,75 @@ public void replaySimple() throws IOException
assertEquals(cellCount, replayer.cells);
}
+ @Test
+ public void replayWithBadSyncMarkerCRC() throws IOException
+ {
+ ColumnFamilyStore cfs =
Keyspace.open(KEYSPACE1).getColumnFamilyStore(STANDARD1);
+
+ Mutation rm2 = new RowUpdateBuilder(cfs.metadata(), 0,
"k2").clustering("bytes")
+
.add("val", bytes("this is a string"))
+ .build();
+ CommitLog.instance.add(rm2);
+ CommitLog.instance.sync(true);
+
+ List<String> activeSegments =
CommitLog.instance.getActiveSegmentNames();
+ assertFalse(activeSegments.isEmpty());
+
+ File directory = new
File(CommitLog.instance.segmentManager.storageDirectory);
+ File firstActiveFile =
Objects.requireNonNull(directory.listFiles((file, name) ->
activeSegments.contains(name)))[0];
+ zeroFirstSyncMarkerCRC(firstActiveFile);
+
+ CommitLogSegmentReader.setAllowSkipSyncMarkerCrc(true);
+
+
Review comment:
nit: extra line
--
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]
