CASSANDRA-18747-4.1: Fix race condition in Schema #2713
Conversation
jacek-lewandowski
changed the title
src/java/org/apache/cassandra/cql3/statements/DescribeStatement.java
Outdated
Show resolved
Hide resolved
|
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. |
|
I don't think 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. |
|
^Couldn't you inject a sleep + getVersionUnsafe() + getVersion() to test the former is out of sync and the latter is safe to use? |
|
So you mean that piece of code in |
|
It seems like |
|
Yeah, I know, I'll run repeated test for 4.1 today |
|
^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 |
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java
Outdated
Show resolved
Hide resolved
|
+1 pending increasing timeouts |
|
is it ok now? |
|
+1 now we need the rest of branches and CI :-) |
jacek-lewandowski
force-pushed
the
CASSANDRA-18747-4.1
branch
from
ba8f4e4
to
29b21c5
Compare
There was a problem hiding this comment.
Left some nit picks.
Looks correct but I have one concern, while DescribeStatement will be improved and this could be considered a bug, we are changing behavior in a patch version and incurring synchronization cost. No? Also, wasn't this working that way even before 4.1?
An explicit test for the new holder is missing, but we will see the Jenkins failures disappearing after commit. I believe that is all we need in this case.
CI LGTM
My comments on the PR to be considered for all branches (except the 4.1 explicit one around change of behavior)
test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java
Outdated
Show resolved
Hide resolved
jacek-lewandowski
force-pushed
the
CASSANDRA-18747-4.1
branch
2 times, most recently
from
bf2d127
to
550d8e9
Compare
| @@ -246,9 +253,10 @@ private Keyspace maybeRemoveKeyspaceInstance(String keyspaceName, Consumer<Keysp | |||
| } | |||
| } | |||
|
|
|||
| @Deprecated(since = "4.1", forRemoval = true) | |||
There was a problem hiding this comment.
I think that you can simply remove it.
| @@ -770,12 +770,12 @@ public static Stream<Keyspace> allExisting() | |||
|
|
|||
| public static Iterable<Keyspace> nonSystem() | |||
There was a problem hiding this comment.
This one seems to be use only in Keyspace.drain where we should use nonLocalStrategy() so we should be able to remove it.
| @@ -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()) | |||
There was a problem hiding this comment.
It seems that we are lying for TableMetrics too :-(
There was a problem hiding this comment.
can we address that in 5.0 and change metrics a bit?
There was a problem hiding this comment.
It was just a statement. We can let that for later.
jacek-lewandowski
force-pushed
the
CASSANDRA-18747-4.1
branch
from
9221c8d
to
dec0d69
Compare
Fixed the inconsistency between distributedKeyspaces and distributedAndLocalKeyspaces
jacek-lewandowski
force-pushed
the
CASSANDRA-18747-4.1
branch
from
dec0d69
to
c10fbc4
Compare
No description provided.