Periodic commit log #3343

belliottsmith · 2024-05-31T20:55:21Z

Thanks for sending a pull request! Here are some tips if you're new here:

Ensure you have added or run the appropriate tests for your PR.
Be sure to keep the PR description updated to reflect all changes.
Write your PR title to summarize what this PR proposes.
If possible, provide a concise example to reproduce the issue for a faster review.
Read our contributor guidelines
If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

dcapwell · 2024-05-31T21:08:03Z

src/java/org/apache/cassandra/journal/ActiveSegment.java

 lastFlushedOffset = allocatePosition;
+ lastFsyncOffsetUpdater.accumulateAndGet(this, allocatePosition, Math::max);


is this correct? if fsync == false then we didn't fsync... so why update the pointer?

good catch, so much for late night updates (when will I learn)

dcapwell · 2024-05-31T21:16:05Z

src/java/org/apache/cassandra/service/accord/serializers/CommandSerializers.java

@@ -99,6 +100,13 @@ public void serialize(T ts, DataOutputPlus out, int version) throws IOException
 TopologySerializers.nodeId.serialize(ts.node, out, version);
 }

+ public void serialize(T ts, DataOutputPlus out) throws IOException


why include ones without a version? what caused this change?

because we have some other callers that don't currently have a version, so easier to invoke ones without a version

dcapwell · 2024-05-31T21:20:33Z

test/distributed/org/apache/cassandra/distributed/test/TestBaseImpl.java

@@ -81,6 +84,7 @@ public void afterEach() {
 @BeforeClass
 public static void beforeClass() throws Throwable
 {
+ CassandraRelevantProperties.SIMULATOR_STARTED.setString(Long.toString(MILLISECONDS.toSeconds(currentTimeMillis())));


did you mean to add this?

Yes, it's for log.xml compatibility

dcapwell · 2024-05-31T21:21:13Z

test/distributed/org/apache/cassandra/distributed/test/accord/AccordLoadTest.java

@@ -43,20 +55,35 @@ public class AccordLoadTest extends AccordTestBase
 @BeforeClass
 public static void setUp() throws IOException
 {
- AccordTestBase.setupCluster(builder -> builder.withConfig(config -> config.set("lwt_strategy", "accord").set("non_serial_write_strategy", "accord")), 2);
+ CassandraRelevantProperties.SIMULATOR_STARTED.setString(Long.toString(MILLISECONDS.toSeconds(currentTimeMillis())));


why are you setting this? we don't setup simulator for jvm-dtest

the simulator log file is the most suitable for running this, and this exists only to setup logging for the instances for that file

dcapwell · 2024-05-31T21:24:21Z

test/distributed/org/apache/cassandra/distributed/test/accord/AccordLoadTest.java

- }, "UPDATE " + qualifiedTableName + " SET v += 1 WHERE k = ? IF EXISTS;", ConsistencyLevel.SERIAL, ConsistencyLevel.QUORUM, random.nextInt(keyCount));
+ if (random.nextFloat() < readChance)
+ {
+ coordinator.executeWithResult((success, fail) -> {


TIL; Future<?> executeWithResult(BiConsumer<SimpleQueryResult, Throwable> callback, String query, ConsistencyLevel serialConsistencyLevel, ConsistencyLevel commitConsistencyLevel, Object... boundValues);

heh...

test/distributed/org/apache/cassandra/distributed/test/accord/AccordLoadTest.java

dcapwell · 2024-05-31T21:31:11Z

test/simulator/main/org/apache/cassandra/simulator/paxos/PairOfSequencesAccordSimulation.java

@@ -149,7 +149,7 @@ public PairOfSequencesAccordSimulation(SimulatedSystems simulated,
 @Override
 protected String createTableStmt()
 {
- return "CREATE TABLE " + KEYSPACE + ".tbl (pk int, count int, seq text, PRIMARY KEY (pk))";
+ return "CREATE TABLE " + KEYSPACE + ".tbl (pk int, count int, seq text, PRIMARY KEY (pk)) WITH transactional_mode = 'full'";


"should" this use transactionMode since you are trying to pass that around now?

fair, I should pass this through yes

dcapwell · 2024-05-31T21:32:07Z

test/simulator/main/org/apache/cassandra/simulator/ClusterSimulation.java

@@ -198,7 +198,7 @@ public static abstract class Builder<S extends Simulation>
 protected HeapPool.Logged.Listener memoryListener;
 protected SimulatedTime.Listener timeListener = (i1, i2) -> {};
 protected LongConsumer onThreadLocalRandomCheck;
- protected String lwtStrategy = "migration";
+ protected String transactionMode = "migration";


did you only keep this so paxos running accord doesn't change topologies? I deleted in my branch but I am working on trying to get topology changes working,

I think lwtStrategy doesn't exist anymore? I was probably doing the bare minimum to get it working

ifesdjeen

I could not spot any problems with this, but I feel like we are severely lacking tests here. I will make it my priority to add at least some test coverage some time soon, as the complexity of these classes grows rather quickly.

ifesdjeen · 2024-06-03T10:51:15Z