You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "iamaleksey (via GitHub)" <gi...@apache.org> on 2026/09/09 14:26:31 UTC
Re: [PR] CASSANDRA-21198 Durably persist timestamps to maintain monotonicity on restart [cassandra]
iamaleksey commented on code in PR #4959:
URL: https://github.com/apache/cassandra/pull/4959#discussion_r3969474015
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -20,32 +20,75 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.zip.CRC32;
+
+import com.google.common.primitives.Longs;
import org.apache.cassandra.io.util.File;
import org.apache.cassandra.io.util.FileInputStreamPlus;
import org.apache.cassandra.io.util.FileOutputStreamPlus;
import org.apache.cassandra.utils.NativeLibrary;
import static org.apache.cassandra.config.DatabaseDescriptor.getAccordJournalDirectory;
+import static org.apache.cassandra.utils.Crc.crc32;
public class ReplayMarkers
{
public static File startMarker()
{
- return new File(getAccordJournalDirectory(), "started");
+ return new File(getAccordJournalDirectory(), "start.crc");
Review Comment:
`start.crc` and `stop.crc` should probably be defined as string constants (used multiple times). But I'd also call them something else. Usually if you have a .crc file you'd assume it contains a checksum of something. Here the files contain the actual something and its checksum. I'd probably choose something along the lines of `started.marker` and `stopped.marker` for clarity.
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -20,32 +20,75 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.zip.CRC32;
+
+import com.google.common.primitives.Longs;
import org.apache.cassandra.io.util.File;
import org.apache.cassandra.io.util.FileInputStreamPlus;
import org.apache.cassandra.io.util.FileOutputStreamPlus;
import org.apache.cassandra.utils.NativeLibrary;
import static org.apache.cassandra.config.DatabaseDescriptor.getAccordJournalDirectory;
+import static org.apache.cassandra.utils.Crc.crc32;
public class ReplayMarkers
{
public static File startMarker()
{
- return new File(getAccordJournalDirectory(), "started");
+ return new File(getAccordJournalDirectory(), "start.crc");
}
public static File safeStopMarker()
{
- return new File(getAccordJournalDirectory(), "stopped");
+ return new File(getAccordJournalDirectory(), "stop.crc");
+ }
+
+ public static class ReplayMarkerMetadata
+ {
+ public final long segmentId;
+ public final long lastUniqueTimeStamp;
+
+ public ReplayMarkerMetadata(long segmentId, long lastUniqueTimeStamp)
+ {
+ this.segmentId = segmentId;
+ this.lastUniqueTimeStamp = lastUniqueTimeStamp;
+ }
+
+ public long getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public long getLastUniqueTimeStamp()
+ {
+ return lastUniqueTimeStamp;
+ }
+
+ @Override
+ public boolean equals(Object other)
Review Comment:
I don't see us using this `equals()` anywhere, including in tests, so we might as well get rid of this dead code. C* convention is to only implement `equals()` + `hashCode()` if we use them somewhere - most commonly as map keys.
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -20,32 +20,75 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.zip.CRC32;
+
+import com.google.common.primitives.Longs;
import org.apache.cassandra.io.util.File;
import org.apache.cassandra.io.util.FileInputStreamPlus;
import org.apache.cassandra.io.util.FileOutputStreamPlus;
import org.apache.cassandra.utils.NativeLibrary;
import static org.apache.cassandra.config.DatabaseDescriptor.getAccordJournalDirectory;
+import static org.apache.cassandra.utils.Crc.crc32;
public class ReplayMarkers
{
public static File startMarker()
{
- return new File(getAccordJournalDirectory(), "started");
+ return new File(getAccordJournalDirectory(), "start.crc");
}
public static File safeStopMarker()
{
- return new File(getAccordJournalDirectory(), "stopped");
+ return new File(getAccordJournalDirectory(), "stop.crc");
+ }
+
+ public static class ReplayMarkerMetadata
Review Comment:
Another nit: this isn't really marker metadata, this is actual marker data.
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -20,32 +20,75 @@
import java.io.IOException;
import java.io.UncheckedIOException;
+import java.util.zip.CRC32;
+
+import com.google.common.primitives.Longs;
import org.apache.cassandra.io.util.File;
import org.apache.cassandra.io.util.FileInputStreamPlus;
import org.apache.cassandra.io.util.FileOutputStreamPlus;
import org.apache.cassandra.utils.NativeLibrary;
import static org.apache.cassandra.config.DatabaseDescriptor.getAccordJournalDirectory;
+import static org.apache.cassandra.utils.Crc.crc32;
public class ReplayMarkers
{
public static File startMarker()
{
- return new File(getAccordJournalDirectory(), "started");
+ return new File(getAccordJournalDirectory(), "start.crc");
}
public static File safeStopMarker()
{
- return new File(getAccordJournalDirectory(), "stopped");
+ return new File(getAccordJournalDirectory(), "stop.crc");
+ }
+
+ public static class ReplayMarkerMetadata
+ {
+ public final long segmentId;
+ public final long lastUniqueTimeStamp;
+
+ public ReplayMarkerMetadata(long segmentId, long lastUniqueTimeStamp)
+ {
+ this.segmentId = segmentId;
+ this.lastUniqueTimeStamp = lastUniqueTimeStamp;
+ }
+
+ public long getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public long getLastUniqueTimeStamp()
+ {
+ return lastUniqueTimeStamp;
+ }
+
+ @Override
+ public boolean equals(Object other)
+ {
+ if (other == this)
+ return true;
+ if (!(other instanceof ReplayMarkerMetadata))
+ return false;
+
+ ReplayMarkerMetadata that = (ReplayMarkerMetadata) other;
+ return this.getSegmentId() == that.getSegmentId() && this.getLastUniqueTimeStamp() == that.getLastUniqueTimeStamp();
+ }
}
- // TODO (required): add checksummed version and default to this (but support unchecksummed for manual editing)
- static void writeMarker(File file, long timestamp)
+ public static void writeMarker(File file, long timestamp, long lastUniqueTimeStamp)
{
try (FileOutputStreamPlus out = new FileOutputStreamPlus(file))
{
- out.writeBytes(Long.toString(timestamp));
+ CRC32 crc = crc32();
+ out.writeLong(timestamp);
Review Comment:
Here and on line 87 should be using `FBUtilities.updateChecksumLong()` instead. It doesn't allocate an array (which in this case, honestly, doesn't matter much), but more importantly is more C*-idiomatic.
##########
src/java/org/apache/cassandra/service/accord/journal/AccordJournal.java:
##########
@@ -623,13 +623,13 @@ public RangeSearcher rangeSearcher()
public void writeStartMarker()
{
- writeMarker(startMarker(), segments.peekSegmentId());
+ writeMarker(startMarker(), segments.peekSegmentId(), -1L);
}
- public void writeSafeStopMarker()
+ public void writeSafeStopMarker(long lastUniqueTimeStamp)
Review Comment:
Here and elsewhere in the patch - use `Timestamp`, not `TimeStamp` capitalisation. The former is used universally throughout the codebase.
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -54,27 +97,59 @@ static void writeMarker(File file, long timestamp)
trySyncJournalDirectory();
}
- public static long readStartMarker()
+ public static ReplayMarkerMetadata readStartMarker()
+ {
+ File crcFile = new File(getAccordJournalDirectory(), "start.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "started"));
+ }
+
+ public static ReplayMarkerMetadata readStopMarker()
{
- return readMarker(startMarker());
+ File crcFile = new File(getAccordJournalDirectory(), "stop.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "stopped"));
}
- public static long readStopMarker()
+ public static ReplayMarkerMetadata readCrcMarker(File file)
{
- return readMarker(safeStopMarker());
+ if (!file.exists())
+ return new ReplayMarkerMetadata(-1L, -1L);
+
+ try (FileInputStreamPlus in = new FileInputStreamPlus(file))
+ {
+ CRC32 crc = crc32();
+ long timestamp = in.readLong();
+ crc.update(Longs.toByteArray(timestamp));
Review Comment:
See the comments for `writeMarker()`
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -54,27 +97,59 @@ static void writeMarker(File file, long timestamp)
trySyncJournalDirectory();
}
- public static long readStartMarker()
+ public static ReplayMarkerMetadata readStartMarker()
+ {
+ File crcFile = new File(getAccordJournalDirectory(), "start.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "started"));
+ }
+
+ public static ReplayMarkerMetadata readStopMarker()
{
- return readMarker(startMarker());
+ File crcFile = new File(getAccordJournalDirectory(), "stop.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "stopped"));
}
- public static long readStopMarker()
+ public static ReplayMarkerMetadata readCrcMarker(File file)
{
- return readMarker(safeStopMarker());
+ if (!file.exists())
+ return new ReplayMarkerMetadata(-1L, -1L);
+
+ try (FileInputStreamPlus in = new FileInputStreamPlus(file))
+ {
+ CRC32 crc = crc32();
+ long timestamp = in.readLong();
+ crc.update(Longs.toByteArray(timestamp));
+ long lastUniqueTimeStamp = in.readLong();
+ crc.update(Longs.toByteArray(lastUniqueTimeStamp));
+ int checksum = in.readInt();
+ if (in.read() != -1 || (int) crc.getValue() != checksum)
+ throw new IOException("Stop.crc file corrupted");
Review Comment:
This won't always be `stop.crc`, as the same method is used for reading the start marker as well.
##########
src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java:
##########
@@ -54,27 +97,59 @@ static void writeMarker(File file, long timestamp)
trySyncJournalDirectory();
}
- public static long readStartMarker()
+ public static ReplayMarkerMetadata readStartMarker()
+ {
+ File crcFile = new File(getAccordJournalDirectory(), "start.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "started"));
+ }
+
+ public static ReplayMarkerMetadata readStopMarker()
{
- return readMarker(startMarker());
+ File crcFile = new File(getAccordJournalDirectory(), "stop.crc");
+ if (crcFile.exists())
+ return readCrcMarker(crcFile);
+ else
+ return readMarker(new File(getAccordJournalDirectory(), "stopped"));
}
- public static long readStopMarker()
+ public static ReplayMarkerMetadata readCrcMarker(File file)
{
- return readMarker(safeStopMarker());
+ if (!file.exists())
+ return new ReplayMarkerMetadata(-1L, -1L);
+
+ try (FileInputStreamPlus in = new FileInputStreamPlus(file))
+ {
+ CRC32 crc = crc32();
+ long timestamp = in.readLong();
+ crc.update(Longs.toByteArray(timestamp));
+ long lastUniqueTimeStamp = in.readLong();
+ crc.update(Longs.toByteArray(lastUniqueTimeStamp));
+ int checksum = in.readInt();
+ if (in.read() != -1 || (int) crc.getValue() != checksum)
+ throw new IOException("Stop.crc file corrupted");
Review Comment:
And I don't think this exception handling ultimately feeds into the defined `StopMarkerFailurePolicy`, instead crashing hard, unconditionally, which is probably undesirable.
--
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