beobal commented on code in PR #3045:
URL: https://github.com/apache/cassandra/pull/3045#discussion_r1476280500
##########
src/java/org/apache/cassandra/tcm/log/LocalLog.java:
##########
@@ -561,15 +586,65 @@ private void notifyPostCommit(ClusterMetadata before,
ClusterMetadata after, boo
ScheduledExecutors.optionalTasks.submit(() ->
listener.notifyPostCommit(before, after, fromSnapshot));
}
+ /**
+ * Essentially same as `ready` but throws an unchecked exception
+ */
+ @VisibleForTesting
+ public final ClusterMetadata readyForTests()
Review Comment:
convention for this is `xxxUnchecked()`
##########
src/java/org/apache/cassandra/tcm/Startup.java:
##########
@@ -141,18 +133,16 @@ public static void initializeAsFirstCMSNode()
public static void initializeAsNonCmsNode(Function<Processor, Processor>
wrapProcessor) throws StartupException
{
- LocalLog.LogSpec logSpec = new
LocalLog.LogSpec().withStorage(LogStorage.SystemKeyspace)
-
.withDefaultListeners();
+ LocalLog.LogSpec logSpec = LocalLog.logSpec()
+
.withStorage(LogStorage.SystemKeyspace)
+
.afterReplay(Startup::scrubDataDirectories)
Review Comment:
just checking I understood correctly, is the benefit of delegating this to
the `LocalLog` that it can be run before any initial listeners are triggered in
`ready()`?
##########
src/java/org/apache/cassandra/tcm/log/LocalLog.java:
##########
@@ -96,25 +98,59 @@ public abstract class LocalLog implements Closeable
// notification to them all.
private final AtomicBoolean replayComplete = new AtomicBoolean();
- public static class LogSpec
+ public static LogSpec logSpec()
{
- public enum WhenReady { NONE, PRE_COMMIT_ONLY, POST_COMMIT_ONLY, ALL};
+ return new LogSpec();
+ }
- private ClusterMetadata initial = new
ClusterMetadata(DatabaseDescriptor.getPartitioner());
+ public static class LogSpec
+ {
+ private ClusterMetadata initial;
+ private Startup.AfterReplay afterReplay = (metadata) -> {};
private LogStorage storage = LogStorage.None;
+ private boolean async = true;
private boolean defaultListeners = false;
- private boolean reset = false;
- private WhenReady whenReady = WhenReady.POST_COMMIT_ONLY;
+ private boolean initializeKeyspaceInstances = true;
+ private boolean isReset = false;
+ private boolean loadSSTables = true;
private final Set<LogListener> listeners = new HashSet<>();
private final Set<ChangeListener> changeListeners = new HashSet<>();
private final Set<ChangeListener.Async> asyncChangeListeners = new
HashSet<>();
+ private LogSpec()
+ {
+ }
+
+ public LogSpec syncForTests()
+ {
+ this.async = false;
+ return this;
+ }
+
+ public LogSpec async()
+ {
+ this.async = true;
+ return this;
+ }
+
public LogSpec withDefaultListeners()
{
return withDefaultListeners(true);
}
+ public LogSpec initializeKeyspaceInstances(boolean
initializeKeyspaceInstances)
Review Comment:
This seems only to be used in conjunction with `withDefaultListeners` - i.e.
error if this is true but the default listeners are not added and vice versa.
Is it just an extra layer of validation to check the caller's intent or is
there something else it provides?
##########
src/java/org/apache/cassandra/tcm/log/LogStorage.java:
##########
@@ -18,12 +18,16 @@
package org.apache.cassandra.tcm.log;
+import com.google.common.annotations.VisibleForTesting;
+
import org.apache.cassandra.tcm.Epoch;
public interface LogStorage extends LogReader
{
void append(long period, Entry entry);
LogState getLogState(Epoch since);
+ @VisibleForTesting
+ void truncate();
Review Comment:
The only place which calls this already hardcodes the instance as
`LogStorage.SystemKeyspace` so we could just call cast it or even inline the
truncation there (`ServerTestUtils::recreateCMS`)
##########
test/unit/org/apache/cassandra/service/ClientWarningsTest.java:
##########
@@ -74,12 +74,12 @@ public void testUnloggedBatch() throws Exception
{
client.connect(false);
- QueryMessage query = new QueryMessage(createBatchStatement2(1),
QueryOptions.DEFAULT);
- Message.Response resp = client.execute(query);
+ Message.Response resp = executeWithRetries(client,
Review Comment:
Is this specific to this patch or general improvements?
##########
test/unit/org/apache/cassandra/hints/HintsUpgradeTest.java:
##########
@@ -47,6 +48,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+@Ignore("TODO: TCM")
Review Comment:
Is this newly failing?
##########
src/java/org/apache/cassandra/tcm/log/LocalLog.java:
##########
@@ -96,25 +98,59 @@ public abstract class LocalLog implements Closeable
// notification to them all.
private final AtomicBoolean replayComplete = new AtomicBoolean();
- public static class LogSpec
+ public static LogSpec logSpec()
{
- public enum WhenReady { NONE, PRE_COMMIT_ONLY, POST_COMMIT_ONLY, ALL};
+ return new LogSpec();
+ }
- private ClusterMetadata initial = new
ClusterMetadata(DatabaseDescriptor.getPartitioner());
+ public static class LogSpec
+ {
+ private ClusterMetadata initial;
+ private Startup.AfterReplay afterReplay = (metadata) -> {};
private LogStorage storage = LogStorage.None;
+ private boolean async = true;
private boolean defaultListeners = false;
- private boolean reset = false;
- private WhenReady whenReady = WhenReady.POST_COMMIT_ONLY;
+ private boolean initializeKeyspaceInstances = true;
+ private boolean isReset = false;
+ private boolean loadSSTables = true;
private final Set<LogListener> listeners = new HashSet<>();
private final Set<ChangeListener> changeListeners = new HashSet<>();
private final Set<ChangeListener.Async> asyncChangeListeners = new
HashSet<>();
+ private LogSpec()
+ {
+ }
+
+ public LogSpec syncForTests()
Review Comment:
should we rename to `syncForTestsAndTools()` or just to `sync()` and add
javadoc to explain intended usages?
##########
src/java/org/apache/cassandra/tcm/ClusterMetadata.java:
##########
@@ -817,6 +818,15 @@ public static ClusterMetadata currentNullable()
return service.metadata();
}
+ public static Optional<ClusterMetadata> currentOptional()
+ {
+ ClusterMetadataService service = ClusterMetadataService.instance();
Review Comment:
> removed in favour of the nullable version.
doesn't look like this is pushed?
--
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]
