krummas commented on code in PR #3045:
URL: https://github.com/apache/cassandra/pull/3045#discussion_r1472844914
##########
src/java/org/apache/cassandra/tcm/ClusterMetadataService.java:
##########
@@ -243,13 +243,24 @@ public static void initializeForTools(boolean
loadSSTables)
{
if (instance != null)
return;
- ClusterMetadata emptyFromSystemTables =
emptyWithSchemaFromSystemTables(Collections.singleton("DC1"));
-
emptyFromSystemTables.schema.initializeKeyspaceInstances(DistributedSchema.empty(),
loadSSTables);
- emptyFromSystemTables = emptyFromSystemTables.forceEpoch(Epoch.EMPTY);
- LocalLog.LogSpec logSpec = new
LocalLog.LogSpec().withInitialState(emptyFromSystemTables)
- .withStorage(new
AtomicLongBackedProcessor.InMemoryStorage());
- LocalLog log = LocalLog.sync(logSpec);
- log.ready();
+ ClusterMetadata emptyFromSystemTables =
emptyWithSchemaFromSystemTables(Collections.singleton("DC1"))
+ .forceEpoch(Epoch.EMPTY);
+
+ LocalLog.LogSpec logSpec = LocalLog.logSpec()
+
.withInitialState(emptyFromSystemTables)
+ .loadSSTables(loadSSTables)
+ .initializeKeyspaceInstances(false)
+ .withDefaultListeners(false)
+ .withListener(new
SchemaListener(loadSSTables) {
Review Comment:
why do we add this empty listener? Isn't withDefaultListeners(false) above
enough?
##########
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:
should we migrate any `currentNullable()` uses to this instead? Or just use
`currentNullable` in `scanLogForPeriod`? (which will soon be gone anyway)
##########
test/distributed/org/apache/cassandra/distributed/test/log/ClusterMetadataTestHelper.java:
##########
@@ -119,13 +121,16 @@ public static void setInstanceForTest()
public static ClusterMetadataService instanceForTest()
{
Review Comment:
this method is unused
##########
src/java/org/apache/cassandra/tcm/Startup.java:
##########
@@ -298,25 +294,20 @@ public static void reinitializeWithClusterMetadata(String
fileName, Function<Pro
if (!metadata.isCMSMember(FBUtilities.getBroadcastAddressAndPort()))
throw new IllegalStateException("When reinitializing with cluster
metadata, we must be in the CMS");
+
// can use local dc here since we know local host in the cms:
- ClusterMetadata emptyFromSystemTables =
emptyWithSchemaFromSystemTables(Collections.singleton(DatabaseDescriptor.getLocalDataCenter()));
- metadata.schema.initializeKeyspaceInstances(DistributedSchema.empty());
- metadata = metadata.forceEpoch(metadata.epoch.nextEpoch());
Review Comment:
Don't we need this anymore?
IIRC the reasoning when adding it was that someone dumps the metadata at
epoch X - then, when we force it here, it should always be a new epoch
##########
src/java/org/apache/cassandra/tcm/Startup.java:
##########
@@ -67,6 +50,15 @@
import org.apache.cassandra.tcm.transformations.UnsafeJoin;
import org.apache.cassandra.tcm.transformations.cms.Initialize;
import org.apache.cassandra.utils.FBUtilities;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
Review Comment:
nit; not sure why these were moved, but code style says that java.* imports
go first
##########
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()
+ {
+ try
+ {
+ return ready();
+ }
+ catch (StartupException e)
+ {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public ClusterMetadata ready() throws StartupException
+ {
+ ClusterMetadata metadata = replayPersisted();
+ spec.afterReplay.accept(metadata);
+ logger.info("Marking LocalLog ready at epoch {}", metadata.epoch);
+
+ // Make sure SchemaListener is added if keyspace instance
initialization is requested
+ if (spec.initializeKeyspaceInstances && !spec.defaultListeners)
+ throw new IllegalStateException("Keyspace initialization
requested, but default listeners are not added.");
+ if (!spec.initializeKeyspaceInstances && spec.defaultListeners)
+ throw new IllegalStateException("Keyspace initialization is
disabled, but default listeners are used: they will initialize the keyspace.");
+
+ if (!replayComplete.compareAndSet(false, true))
+ throw new IllegalStateException("Log is already fully
initialised");
+
+ logger.debug("Marking LocalLog ready at epoch {}",
committed.get().epoch);
+ if (spec.defaultListeners)
+ {
+ logger.info("Adding default listeners to LocalLog");
+ addListeners();
+ }
+ else
+ {
+ logger.info("Adding specified listeners to LocalLog");
+ spec.listeners.forEach(this::addListener);
+ spec.changeListeners.forEach(this::addListener);
+ spec.asyncChangeListeners.forEach(this::addListener);
+ }
+
+ logger.info("Notifying all registered listeners of both pre and post
commit event");
+ notifyListeners(spec.initial);
Review Comment:
in the LocalLog constructor we set committed to spec.initial - so here I
think we always notifyListeners with the same prev/next metadatas - at least in
the reset case we should always notify from an empty cluster metadata I think?
looks like we handle this for schema with that new `isFirst` boolean but I
think we need it for all listeners (say we change the state of a nodeid from
registered to joined - it wouldn't get properly added to gossip (I think?))
--
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]
