[GitHub] [cassandra] adelapena commented on a diff in pull request #1678: Cassandra 17658

[GitBox <gi...@apache.org>]

adelapena commented on code in PR #1678:
URL: https://github.com/apache/cassandra/pull/1678#discussion_r905941912


##########
src/java/org/apache/cassandra/db/Keyspace.java:
##########
@@ -137,7 +140,10 @@ public static void setInitialized()
     @VisibleForTesting
     public static void unsetInitialized()
     {
-        initialized = false;
+        synchronized (Schema.instance)

Review Comment:
   I think the method `Kesypace.unsetInitialized` isn't used anywhere, and it didn't seem to be used when it was introduced for the first time on CASSANDRA-17044. Should we remove it?



##########
test/long/org/apache/cassandra/cql3/CorruptionTest.java:
##########
@@ -59,7 +60,11 @@ public class CorruptionTest extends SchemaLoader
     @BeforeClass()
     public static void setup() throws ConfigurationException, IOException
     {
-        Schema.instance.clear();
+        DatabaseDescriptor.daemonInitialization();
+
+        // clean-up is only needed if we run the test from IDE
+        ServerTestUtils.mkdirs();
+        ServerTestUtils.cleanup();

Review Comment:
   This is always done before any instantiation of `EmbeddedCassandraService`, so we may make tests slightly briefer and less error prone if we encapsulate both things into a single utility method. Going a bit further, we could move the entire `EmbeddedCassandraService` class into the tests directory (it's only used for  tests), and place the cleanup of dirs into its constructor. wdyt?



##########
test/long/org/apache/cassandra/cql3/CorruptionTest.java:
##########
@@ -59,7 +60,11 @@ public class CorruptionTest extends SchemaLoader
     @BeforeClass()
     public static void setup() throws ConfigurationException, IOException
     {
-        Schema.instance.clear();

Review Comment:
   Nit: removing this leaves an unused import of `Schema`.



##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -227,6 +227,7 @@ public Keyspace maybeAddKeyspaceInstance(String keyspaceName, Supplier<Keyspace>
         return keyspaceInstances.blockingLoadIfAbsent(keyspaceName, loadFunction);
     }
 
+    @VisibleForTesting

Review Comment:
   The method `Schema.maybeRemoveKeyspaceInstance` doesn't seem used anymore, since it has lost its single caller, `Schema.dropKeyspace`. Should we remove it?



-- 
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