You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "jacek-lewandowski (via GitHub)" <gi...@apache.org> on 2023/09/21 09:15:36 UTC
[GitHub] [cassandra] jacek-lewandowski opened a new pull request, #2713: Cassandra 18747 4.1
jacek-lewandowski opened a new pull request, #2713:
URL: https://github.com/apache/cassandra/pull/2713
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1744728841
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/955/workflows/dee3d689-6795-4089-bb78-539b837bdb1f
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/961/workflows/d8e74ab8-f3d8-404f-9ca7-df4a0ccbde29
--
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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1738514143
+1 pending increasing timeouts
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1735105765
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/939/workflows/d6ef66f0-18b6-420d-a264-6cc6932ac0ae
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1351192250
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,27 +509,23 @@ 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 getVersion()
+ public UUID getVersionUnsafe()
{
return version;
}
/**
- * Checks whether the given schema version is the same as the current local schema.
+ * 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 boolean isSameVersion(UUID schemaVersion)
+ public synchronized UUID getVersion()
{
- return schemaVersion != null && schemaVersion.equals(version);
- }
-
- /**
- * Checks whether the current schema is empty.
- */
- public boolean isEmpty()
Review Comment:
I do not recommend removing public methods on patch release
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1335484971
##########
src/java/org/apache/cassandra/cql3/statements/DescribeStatement.java:
##########
@@ -132,8 +132,13 @@ public final ResultMessage execute(QueryState state, QueryOptions options, long
@Override
public ResultMessage executeLocally(QueryState state, QueryOptions options)
{
- Keyspaces keyspaces = Schema.instance.distributedAndLocalKeyspaces();
- UUID schemaVersion = Schema.instance.getVersion();
+ UUID schemaVersion;
+ Keyspaces keyspaces;
+ do
+ {
+ schemaVersion = Schema.instance.getVersion();
Review Comment:
So the error was coming from sthg running in between these 2 lines of code getting the schema versions not match?
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1734916527
So you mean that piece of code in `DescriptionStatement` right? yeah, I can try
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1736890910
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/941/workflows/46fc6cb7-135e-4862-b9d3-6996c0993de8
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1738976104
is it ok now?
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339556470
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
Schema cahnge operations can take >60s in jenkins. We have 120s in dtests i.e. and I am raising this to 120s for jvm dtests. We should raise it here as well to prevent flakies.
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1746424588
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/961/workflows/ccb113fc-7a0e-4fa6-bdb9-4c284bd7e97a
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "blerer (via GitHub)" <gi...@apache.org>.
blerer commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1358307839
##########
src/java/org/apache/cassandra/metrics/TableMetrics.java:
##########
@@ -280,7 +280,7 @@ private static Pair<Long, Long> totalNonSystemTablesSize(Predicate<SSTableReader
{
long total = 0;
long filtered = 0;
- for (String keyspace : Schema.instance.getNonSystemKeyspaces().names())
+ for (String keyspace : Schema.instance.distributedKeyspaces().names())
Review Comment:
It seems that we are lying for TableMetrics too :-(
##########
src/java/org/apache/cassandra/db/Keyspace.java:
##########
@@ -770,12 +770,12 @@ public static Stream<Keyspace> allExisting()
public static Iterable<Keyspace> nonSystem()
Review Comment:
This one seems to be use only in Keyspace.drain where we should use `nonLocalStrategy()` so we should be able to remove it.
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -246,9 +253,10 @@ private Keyspace maybeRemoveKeyspaceInstance(String keyspaceName, Consumer<Keysp
}
}
+ @Deprecated(since = "4.1", forRemoval = true)
Review Comment:
I think that you can simply 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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1733174841
Ar you planning on adding a test for the specific scenario? Also you can run the pre-commit CI job rather than the separate one which is more cumbersome and some jobs got missed.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1734212439
I don't I can add a test for this scenario due to changes I've made. To prove that, I needed to add a sleep between updating one and the other field. With those changes such a test makes no sense.
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339951467
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
It does indeed make a lot of sense to raise it. The pain of having to continuously debug flakies, read the logs, chase the code, etc. which in the end happen to be just timeouts it's not negligible and very demotivating imo.
20s sounds like a magic number. You mentioned `test.driver.read_timeout_ms` is at 24s. I wonder if we can access that programmatically?
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "JeremiahDJordan (via GitHub)" <gi...@apache.org>.
JeremiahDJordan commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1346271239
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,27 +509,23 @@ 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 getVersion()
+ public UUID getVersionUnsafe()
{
return version;
}
/**
- * Checks whether the given schema version is the same as the current local schema.
+ * 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 boolean isSameVersion(UUID schemaVersion)
+ public synchronized UUID getVersion()
{
- return schemaVersion != null && schemaVersion.equals(version);
- }
-
- /**
- * Checks whether the current schema is empty.
- */
- public boolean isEmpty()
Review Comment:
Why remove the helper function?
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,27 +509,23 @@ 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 getVersion()
+ public UUID getVersionUnsafe()
{
return version;
}
/**
- * Checks whether the given schema version is the same as the current local schema.
+ * 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 boolean isSameVersion(UUID schemaVersion)
Review Comment:
Why remove the helper function?
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1335529352
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -77,8 +77,19 @@ public class Schema implements SchemaProvider
public static final Schema instance = new Schema();
- private volatile Keyspaces distributedKeyspaces = Keyspaces.none();
- private volatile Keyspaces distributedAndLocalKeyspaces;
+ private static class Holder
+ {
+ private final Keyspaces distributedKeyspaces;
+ private final Keyspaces distributedAndLocalKeyspaces;
+
+ private Holder(Keyspaces distributedKeyspaces, Keyspaces distributedAndLocalKeyspaces)
Review Comment:
This is just to force both to be updated at the same time and avoid them getting out of sync?
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1336214126
##########
src/java/org/apache/cassandra/cql3/statements/DescribeStatement.java:
##########
@@ -132,8 +132,13 @@ public final ResultMessage execute(QueryState state, QueryOptions options, long
@Override
public ResultMessage executeLocally(QueryState state, QueryOptions options)
{
- Keyspaces keyspaces = Schema.instance.distributedAndLocalKeyspaces();
- UUID schemaVersion = Schema.instance.getVersion();
+ UUID schemaVersion;
+ Keyspaces keyspaces;
+ do
+ {
+ schemaVersion = Schema.instance.getVersion();
Review Comment:
The error was not coming from this place - this is a bug I discovered while inspecting the code. Although the update of version and list of keyspaces is synchronized, they are not updated atomically. In particular, while the list of keyspaces is updated incrementally with each added/modified/dropped keyspace, the version is updated at the of the entire schema transformation. It is because we do not know the intermediate schema versions, and also for the performance reasons.
Therefore, this simple loop ensures that the schema did not change while we requested list of keyspaces. In other words, schema version and the keyspaces match.
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1351191700
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,27 +509,23 @@ 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 getVersion()
+ public UUID getVersionUnsafe()
{
return version;
}
/**
- * Checks whether the given schema version is the same as the current local schema.
+ * 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 boolean isSameVersion(UUID schemaVersion)
Review Comment:
I wouldn't remove public function, especially on a patch release.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1336214857
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -77,8 +77,19 @@ public class Schema implements SchemaProvider
public static final Schema instance = new Schema();
- private volatile Keyspaces distributedKeyspaces = Keyspaces.none();
- private volatile Keyspaces distributedAndLocalKeyspaces;
+ private static class Holder
+ {
+ private final Keyspaces distributedKeyspaces;
+ private final Keyspaces distributedAndLocalKeyspaces;
+
+ private Holder(Keyspaces distributedKeyspaces, Keyspaces distributedAndLocalKeyspaces)
Review Comment:
yes, instead of making getter synchronized as it would be slower and lead to deadlocks.
--
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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1740460754
+1 now we need the rest of branches and CI :-)
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1354167856
##########
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:
yes, would you like me to add that to the comment? I believe the API should be sufficient and minimal
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1770431323
j8 - https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/977/workflows/31134b2b-11d9-4992-b2e8-04ea4ed9ef4c 🆗
j11 - https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/977/workflows/48fd2a77-cff7-4bd4-ae7a-a4dd9f824149 🆗
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski closed pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
URL: https://github.com/apache/cassandra/pull/2713
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1354248710
##########
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:
It is a bug if we can return schema and schema version where version does not correspond to the schema, which will result in paging failure; You are right that the bug exists in 4.0 as well; 3.11 and prior do not have server-side `DescribeStatement`. I'll create a separate ticket to fix that starting from 4.0 (https://issues.apache.org/jira/browse/CASSANDRA-18921)
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1357172745
##########
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:
`would you like me to add that to the comment? `
I will leave the final decision to you, just mentioned that it is a bit more than that :-)
--
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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1734911544
^Couldn't you inject a sleep + getVersionUnsafe() + getVersion() to test the former is out of sync and the latter is safe to use?
--
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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1736775145
It seems like `MigrationCoordinatorTest` failed on the repeats. I checked JIRA and Butler and they both seem clean. So it's worth investigating imo
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1736838790
Yeah, I know, I'll run repeated test for 4.1 today
--
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
[GitHub] [cassandra] bereng commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1737182714
^Ok so it failed on 4.1 as well. A ticket for that one needs to be created and I'll get back to this review shortly
--
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
[GitHub] [cassandra] jacek-lewandowski commented on pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1730844989
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/909/workflows/8c169ac2-a284-426b-9a64-510e966e63d5
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339921205
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
changed to 20 seconds, it does not make sense to increase it more for those tests. Also renamed "blablabla" rule 🤦🏻
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
changed to 20 seconds, it does not make sense to increase it more for those tests. Also renamed "blablabla" rule 🤦🏻
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1360389344
##########
src/java/org/apache/cassandra/metrics/TableMetrics.java:
##########
@@ -280,7 +280,7 @@ private static Pair<Long, Long> totalNonSystemTablesSize(Predicate<SSTableReader
{
long total = 0;
long filtered = 0;
- for (String keyspace : Schema.instance.getNonSystemKeyspaces().names())
+ for (String keyspace : Schema.instance.distributedKeyspaces().names())
Review Comment:
can we address that in 5.0 and change metrics a bit?
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "blerer (via GitHub)" <gi...@apache.org>.
blerer commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1360550323
##########
src/java/org/apache/cassandra/metrics/TableMetrics.java:
##########
@@ -280,7 +280,7 @@ private static Pair<Long, Long> totalNonSystemTablesSize(Predicate<SSTableReader
{
long total = 0;
long filtered = 0;
- for (String keyspace : Schema.instance.getNonSystemKeyspaces().names())
+ for (String keyspace : Schema.instance.distributedKeyspaces().names())
Review Comment:
It was just a statement. We can let that for later.
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339623137
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
_jvm_ dtest, not py dtests. And repeated runs worked on circle, not jenkins. I know it sounds crazy but on jenkins schema changes can take >60s easily and sbdy will have to read logs/debug if we don't go with more realistic, though crazy, timeouts :shrug:
--
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
[GitHub] [cassandra] bereng commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339731150
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
Yes I know.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339589770
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
Though, this is not a dtest and there is no networking. Repeated run with this test already completed without problems. If you insist, I can change that, however, I cannot justify such a long timeout given what it waits for.
--
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
[GitHub] [cassandra] jacek-lewandowski commented on a diff in pull request #2713: CASSANDRA-18747-4.1: Fix race condition in Schema
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1339705751
##########
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java:
##########
@@ -377,6 +393,52 @@ 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 = "blablabla",
+ targetClass = "Schema",
+ targetMethod = "distributedAndLocalKeyspaces",
+ targetLocation = "AT ENTRY",
+ action = "org.apache.cassandra.cql3.statements.DescribeStatementTest.awaitDescribeStatementLatch()")
+ public void testNotReturningPartiallyAppliedSchema() throws Throwable
+ {
+ 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(Duration.ofSeconds(5)).until(() -> describeStatementLatch.count() == 1);
Review Comment:
this is cqltester based unit test....
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#issuecomment-1744471952
https://app.circleci.com/pipelines/github/jacek-lewandowski/cassandra/955/workflows/dee3d689-6795-4089-bb78-539b837bdb1f
--
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
Re: [PR] CASSANDRA-18747-4.1: Fix race condition in Schema [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2713:
URL: https://github.com/apache/cassandra/pull/2713#discussion_r1346296286
##########
src/java/org/apache/cassandra/schema/Schema.java:
##########
@@ -496,27 +509,23 @@ 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 getVersion()
+ public UUID getVersionUnsafe()
{
return version;
}
/**
- * Checks whether the given schema version is the same as the current local schema.
+ * 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 boolean isSameVersion(UUID schemaVersion)
+ public synchronized UUID getVersion()
{
- return schemaVersion != null && schemaVersion.equals(version);
- }
-
- /**
- * Checks whether the current schema is empty.
- */
- public boolean isEmpty()
Review Comment:
Keeping them would require safe and unsafe versions of them, which I presumed to be clumsy; I don't have a strong opinion on that though
--
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