ekaterinadimitrova2 commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1351200063
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,54 @@ public void testDescribe() throws Throwable
}
}
+ private static volatile CountDownLatch describeStatementLatch;
+ public static void awaitDescribeStatementLatch()
+ {
+ if (Arrays.stream(Thread.currentThread().getStackTrace()).anyMatch(ste
-> ste.getClassName().equals(DescribeStatement.class.getName())))
+ {
+ describeStatementLatch.decrement();
+ describeStatementLatch.awaitThrowUncheckedOnInterrupt();
+ }
+ }
+
+ @Test
+ @BMRule(name = "await describe statement latch",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action =
"org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ Duration timeout =
Duration.ofMillis(DatabaseDescriptor.getReadRpcTimeout(TimeUnit.MILLISECONDS));
+
+ describeStatementLatch = CountDownLatch.newCountDownLatch(2);
+ ForkJoinTask<ResultSet> describeFuture =
ForkJoinPool.commonPool().submit(() -> {
+ try
+ {
+ return executeNetWithPaging(ProtocolVersion.CURRENT, "DESCRIBE
KEYSPACES", 1);
+ }
+ catch (Throwable e)
+ {
+ throw new RuntimeException(e);
+ }
+ });
+ // wait for the describe statement to be in progress
+ Awaitility.await().atMost(timeout).until(() ->
describeStatementLatch.count() == 1);
+
+ // at this point, the describe statement got the schema version and is
about to get the keyspaces
+ // we will now create a keyspace, release the lock, and let the
describe statement continue
+ String ks = createKeyspace("CREATE KEYSPACE %s WITH REPLICATION =
{'class' : 'SimpleStrategy', 'replication_factor' : 1};");
+ describeStatementLatch.decrement();
+
+ // the paginated describe statement should complete successfully and
should include the new keyspace even though
+ // the keyspace was created after the describe statement started; this
proves that when the describe statement
+ // finds itself in a situation where the schema being changed, it will
wait for completion before actually
+ // starting execution; otherwise we would get an exception
Review Comment:
Isn't this a change of behavior for Describe in a patch version?
While it seems more correct, it is an improvement. Also, I suspect this was
the same even before 4.1, no? Was this checked? Also, there is still
synchronization cost in place, unfortunately, added through the synchronized
getVersion.
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,25 +509,43 @@ public Optional<Function> findFunction(FunctionName name,
List<AbstractType<?>>
/* Version control */
/**
- * @return current schema version
+ * Returns the current schema version. Although, if the schema is being
updated while the method was called, it
+ * can return a stale version which does not correspond to the current
keyspaces metadata. It is because the schema
+ * version is unknown for the partially applied changes and is updated
after the entire schema change is applied.
+ * @see #getVersion()
+ */
+ public UUID getVersionUnsafe()
+ {
+ return version;
+ }
+
+ /**
+ * Returns the curren schema version. If the schema is in the middle of
the update, the method will wait until
+ * the update is completed and return the new version.
*/
- public UUID getVersion()
+ public synchronized UUID getVersion()
{
return version;
}
/**
* Checks whether the given schema version is the same as the current
local schema.
+ *
+ * @deprecated please use {@link #getVersion()} or {@link
#getVersionUnsafe()} depending on the use case
*/
- public boolean isSameVersion(UUID schemaVersion)
+ @Deprecated(since = "4.1", forRemoval = true)
+ public synchronized boolean isSameVersion(UUID schemaVersion)
{
return schemaVersion != null && schemaVersion.equals(version);
}
/**
* Checks whether the current schema is empty.
+ *
Review Comment:
```suggestion
* <p>
```
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,25 +509,43 @@ public Optional<Function> findFunction(FunctionName name,
List<AbstractType<?>>
/* Version control */
/**
- * @return current schema version
+ * Returns the current schema version. Although, if the schema is being
updated while the method was called, it
+ * can return a stale version which does not correspond to the current
keyspaces metadata. It is because the schema
+ * version is unknown for the partially applied changes and is updated
after the entire schema change is applied.
+ * @see #getVersion()
+ */
+ public UUID getVersionUnsafe()
+ {
+ return version;
+ }
+
+ /**
+ * Returns the curren schema version. If the schema is in the middle of
the update, the method will wait until
+ * the update is completed and return the new version.
*/
- public UUID getVersion()
+ public synchronized UUID getVersion()
{
return version;
}
/**
* Checks whether the given schema version is the same as the current
local schema.
+ *
+ * @deprecated please use {@link #getVersion()} or {@link
#getVersionUnsafe()} depending on the use case
*/
- public boolean isSameVersion(UUID schemaVersion)
+ @Deprecated(since = "4.1", forRemoval = true)
+ public synchronized boolean isSameVersion(UUID schemaVersion)
Review Comment:
Technically you replace it with the `equals` method being chained to the
appropriate getVersion method.
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,25 +509,43 @@ public Optional<Function> findFunction(FunctionName name,
List<AbstractType<?>>
/* Version control */
/**
- * @return current schema version
+ * Returns the current schema version. Although, if the schema is being
updated while the method was called, it
+ * can return a stale version which does not correspond to the current
keyspaces metadata. It is because the schema
+ * version is unknown for the partially applied changes and is updated
after the entire schema change is applied.
+ * @see #getVersion()
+ */
+ public UUID getVersionUnsafe()
+ {
+ return version;
+ }
+
+ /**
+ * Returns the curren schema version. If the schema is in the middle of
the update, the method will wait until
Review Comment:
```suggestion
* Returns the current schema version. If the schema is in the middle of
the update, the method will wait until
```
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,25 +509,43 @@ public Optional<Function> findFunction(FunctionName name,
List<AbstractType<?>>
/* Version control */
/**
- * @return current schema version
+ * Returns the current schema version. Although, if the schema is being
updated while the method was called, it
+ * can return a stale version which does not correspond to the current
keyspaces metadata. It is because the schema
+ * version is unknown for the partially applied changes and is updated
after the entire schema change is applied.
+ * @see #getVersion()
+ */
+ public UUID getVersionUnsafe()
+ {
+ return version;
+ }
+
+ /**
+ * Returns the curren schema version. If the schema is in the middle of
the update, the method will wait until
+ * the update is completed and return the new version.
*/
- public UUID getVersion()
+ public synchronized UUID getVersion()
{
return version;
}
/**
* Checks whether the given schema version is the same as the current
local schema.
+ *
Review Comment:
```suggestion
* <p>
```
--
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]
