bereng commented on a change in pull request #1221:
URL: https://github.com/apache/cassandra/pull/1221#discussion_r727719792
##########
File path: src/java/org/apache/cassandra/schema/Schema.java
##########
@@ -85,25 +85,34 @@ private Schema()
*
* See CASSANDRA-16856/16996. Make sure schema pulls are synchronized to
prevent concurrent schema pull/writes
*/
- public static synchronized void saveSystemKeyspace()
+ public static void saveSystemKeyspace()
{
- SchemaKeyspace.saveSystemKeyspacesSchema();
+ synchronized(instance)
+ {
+ SchemaKeyspace.saveSystemKeyspacesSchema();
+ }
}
/**
* See CASSANDRA-16856/16996. Make sure schema pulls are synchronized to
prevent concurrent schema pull/writes
*/
- public static synchronized void truncateSystemKeyspace()
+ public static void truncateSystemKeyspace()
{
- SchemaKeyspace.truncate();
+ synchronized(instance)
+ {
+ SchemaKeyspace.truncate();
+ }
}
/**
* See CASSANDRA-16856/16996. Make sure schema pulls are synchronized to
prevent concurrent schema pull/writes
*/
- public static synchronized Collection<Mutation> schemaKeyspaceAsMutations()
+ public static Collection<Mutation> schemaKeyspaceAsMutations()
{
- return SchemaKeyspace.convertSchemaToMutations();
+ synchronized(instance)
+ {
+ return SchemaKeyspace.convertSchemaToMutations();
+ }
Review comment:
Pushed
##########
File path: src/java/org/apache/cassandra/schema/Schema.java
##########
@@ -85,25 +85,34 @@ private Schema()
*
* See CASSANDRA-16856/16996. Make sure schema pulls are synchronized to
prevent concurrent schema pull/writes
*/
- public static synchronized void saveSystemKeyspace()
+ public static void saveSystemKeyspace()
{
- SchemaKeyspace.saveSystemKeyspacesSchema();
+ synchronized(instance)
+ {
+ SchemaKeyspace.saveSystemKeyspacesSchema();
+ }
}
/**
* See CASSANDRA-16856/16996. Make sure schema pulls are synchronized to
prevent concurrent schema pull/writes
*/
- public static synchronized void truncateSystemKeyspace()
+ public static void truncateSystemKeyspace()
Review comment:
pushed
##########
File path: test/unit/org/apache/cassandra/schema/SchemaKeyspaceTest.java
##########
@@ -183,8 +269,32 @@ public void testSchemaNoColumn()
KeyspaceParams.simple(1),
SchemaLoader.standardCFMD(testKS,
testTable));
// Delete all colmns in the schema
- String query = String.format("DELETE FROM %s.%s WHERE keyspace_name=?
and table_name=?", SchemaConstants.SCHEMA_KEYSPACE_NAME,
SchemaKeyspace.COLUMNS);
+ String query = String.format("DELETE FROM %s.%s WHERE keyspace_name=?
and table_name=?", SchemaConstants.SCHEMA_KEYSPACE_NAME,
SystemKeyspaceConstants.COLUMNS);
executeOnceInternal(query, testKS, testTable);
SchemaKeyspace.fetchNonSystemKeyspaces();
}
+
+ private void testMethodSynchronicity(Runnable method) throws Exception
+ {
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ CountDownLatch pendingStarts = new CountDownLatch(2);
+
+ threadBlock = new CountDownLatch(2);
+ pool.execute(() -> {
+ pendingStarts.countDown();
+ method.run();
+ });
+ pool.execute(() -> {
+ pendingStarts.countDown();
+ method.run();
+ });
+
+ // We have started 2 threads. 1 thread should be held by the latch
threadBlock, the other should be held as
+ // proof of thread safety.
+ pendingStarts.await(10, TimeUnit.SECONDS);
+ Util.spinAssertEquals(1L, () -> threadBlock.getCount(), 10);
+
+ // Do not execute the tasks. Just kill it.
+ pool.shutdownNow();
+ }
Review comment:
@maedhroz I disagree the tests only check the syncs. There is indeed a
test that tests the sync across the former static/instance problem but that's
not the point of that test.
[Here](https://github.com/apache/cassandra/pull/1221/files#diff-08752d813e58a10b673dbf64a9cd3fe619c5a27db20918df58279f7d0040eee0R133)
it tests if a partial update can be seen by some other thread. That is the
important bit that aligns to what we discussed for a test iiuc.
I can add this test but I don't see what value it adds. Also it is time
dependent iiuc and thread may or may not collide. On the other hand it tests
from the highest API whereas I test from the lowest. I will add it if that +1s
it but I still think the other test is better than this one?
--
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]
