CASSANDRA-17819: Fix resetting the schema #1804

jacek-lewandowski · 2022-08-22T06:42:36Z

No description provided.

ekaterinadimitrova2

Left some comments, primarily questions actually. Need to look at it more tomorrow. Seems like there are few things going on and I do not want to change any behavior

ekaterinadimitrova2 · 2022-08-23T21:03:04Z

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

How about some Javadoc?

ekaterinadimitrova2 · 2022-08-23T22:19:43Z

src/java/org/apache/cassandra/schema/DefaultSchemaUpdateHandler.java

Shall we add some logging in case no update has happened?

src/java/org/apache/cassandra/schema/DefaultSchemaUpdateHandler.java

ekaterinadimitrova2 · 2022-08-23T22:49:40Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

Why this was switched to trace?

I'll revert

but actually I considered this message as temporary situation - when the messaging version is eventually known, the schema is either pulled or it is logged that the messaging version is incompatible (at debug level)

ekaterinadimitrova2 · 2022-08-23T22:55:06Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

I think I mentioned in the previous version but for completeness - I think this TO DO deserves to be documented actually

Ok, but where do you want it to be documented? I think we should rather fix it

Then in a follow up ticket in order to be fixed and add the number in this comment? WDYT?

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

ekaterinadimitrova2 · 2022-08-23T23:42:27Z

src/java/org/apache/cassandra/schema/DefaultSchemaUpdateHandler.java

Why did you decide to move it up?

ekaterinadimitrova2 · 2022-08-23T23:46:09Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

Have to think a bit about this TO DO

Still trying to understand why it's needed, but for sure a quick attempt at removing it breaks the tests.

The same logic is in pullComplete.
Seems to me we markReceived before we deal with requestQueue in maybePullSchema, I am still not sure why though.... looking again into that

Actually, it hasn't necessarily marked are received, has it? It's only marked as received if it's equals to the local schema, I think.

Yes, reading back that is right but then it is confusing why in those cases we add to the requestQueue which seems to be a queue of endpoints from which we are going to fetch.... do we need still to fetch? :-)

At that point we do not remove the endpoint from the queue, we seem to remove it next time we call maybePullSchema and we iterate the queue. Not sure why that approach was taken really and not just not adding it at all in case we markReceived... I will experiment

So we add it to requestQueue because we might need to fetch it, unless it has been marked as received. I guess the simplified version would be adding the endpoint to the request queue only if hasn't been marked, something like:

VersionInfo info = versionInfo.computeIfAbsent(version, VersionInfo::new); info.endpoints.add(endpoint); logger.trace("Added endpoint {} to schema {}: {}", endpoint, info.version, info); if (Objects.equals(schemaVersion.get(), version)) { info.markReceived(); logger.trace("Schema {} from {} has been marked as received because it is equal the local schema", version, endpoint); } else { info.requestQueue.addFirst(endpoint); }

Yes, that was my point but I have to test if it can have some other side effects that we might miss...

At least the MigrationCoordinatorTest and SchemaTest (both in-jvm and unit tests) pass for me locally with this change

exactly, that was what I've commented - "...given we've just marked this schema version as received..." :-)

ekaterinadimitrova2 · 2022-08-24T00:16:01Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

nit: endpoints

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

ekaterinadimitrova2

Left some small comments, I want to do one more pass and check further the MigrationCoordinatorTest as I just skimmed through it. I want to be sure I didn't miss anything. I also want to look further in 4.0. Not an area of the code that I have a lot of experience

ekaterinadimitrova2 · 2022-08-29T20:12:47Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

I think we need to make it print the SCHEMA_PULL_INTERVAL as it is mutable, not always 1 minute necessarily anymore

ekaterinadimitrova2 · 2022-08-29T20:15:20Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

immediately

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

ekaterinadimitrova2 · 2022-08-30T00:17:40Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

This one too
Not that I am super opinionated but it just feels like it can improve readability that way

ekaterinadimitrova2 · 2022-08-30T00:57:33Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

I think this can be split in a few lines as it becomes too long

I will, but with the current formatting rules I can see no way to format it nicely

ekaterinadimitrova2 · 2022-08-30T01:01:09Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

This line can also be split in a few lines so we don't have to scroll right

again, the formatting rules does not allow for make it nicely to my knowledge

ekaterinadimitrova2 · 2022-08-30T01:09:50Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

This line also seems too long

ekaterinadimitrova2 · 2022-08-30T01:11:34Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

Unused imports

ekaterinadimitrova2 · 2022-08-30T01:11:50Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

Unused import

adelapena · 2022-08-30T12:47:29Z

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

I'd add a brief JavaDoc line saying what the property does.

adelapena · 2022-08-30T12:48:22Z

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

Probably we should include the unit in the name, like in SCHEMA_PULL_INTERVAL_MS.

adelapena · 2022-08-30T12:48:59Z

src/java/org/apache/cassandra/schema/DefaultSchemaUpdateHandler.java

Suggested change

} catch (RuntimeException ex) {

}

catch (RuntimeException ex)

{

adelapena · 2022-08-30T12:49:26Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

Nit: unused import

adelapena · 2022-08-30T12:50:03Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

Suggested change

* As long as the certain version is advertised by some node, it is being tracked. As long as a version is tracked,

* As long as a certain version is advertised by some node, it is being tracked. As long as a version is tracked,

adelapena · 2022-08-30T12:51:20Z

src/java/org/apache/cassandra/schema/MigrationCoordinator.java

Maybe it should be waitQueueSize?

Suggested change

", waitQueue=" + waitQueue.getWaiting() +

", waitQueueSize=" + waitQueue.getWaiting() +

adelapena · 2022-08-30T12:52:06Z

src/java/org/apache/cassandra/schema/Schema.java

Suggested change

if (!clearCompletion.await(StorageService.SCHEMA_DELAY_MILLIS, TimeUnit.MILLISECONDS)) {

if (!clearCompletion.await(StorageService.SCHEMA_DELAY_MILLIS, TimeUnit.MILLISECONDS))

{

adelapena · 2022-08-30T12:52:51Z

src/java/org/apache/cassandra/schema/SchemaUpdateHandler.java

I think we should either add some description to the @return tag or remove it.

ok, and btw. I suppose the more adequate names for those methods now would be reset -> sync, clear -> refresh

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

jacek-lewandowski · 2022-09-01T12:01:35Z

Thank you for thorough review, I've applied your suggestions

ekaterinadimitrova2 · 2022-09-12T16:21:02Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

typo: syncs

ekaterinadimitrova2 · 2022-09-12T19:31:17Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

I would use the names here probably tbl_one or so.

ekaterinadimitrova2 · 2022-09-12T19:46:03Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

I would probably change this name to checkTablesPropagated or something

ekaterinadimitrova2 · 2022-09-12T19:50:14Z

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java

a definition

Patch by Jacek Lewandowski, reviewed by Andrés de la Peña and Ekaterina Dimitrova for CASSANDRA-17819

jacek-lewandowski · 2022-09-16T08:02:14Z

Merged to cassandra-4.1 in d8bbeb9

jacek-lewandowski mentioned this pull request Aug 22, 2022

Cassandra 17819 #1800

Closed

jacek-lewandowski force-pushed the CASSANDRA-17819-2 branch from f703959 to 3987d38 Compare August 22, 2022 19:04

ekaterinadimitrova2 reviewed Aug 24, 2022

View reviewed changes

jacek-lewandowski force-pushed the CASSANDRA-17819-2 branch from 3987d38 to 6761ff6 Compare August 26, 2022 16:43

ekaterinadimitrova2 reviewed Aug 30, 2022

View reviewed changes

adelapena reviewed Aug 30, 2022

View reviewed changes

jacek-lewandowski force-pushed the CASSANDRA-17819-2 branch from 061eb71 to 214ded9 Compare September 8, 2022 12:22

ekaterinadimitrova2 reviewed Sep 12, 2022

View reviewed changes

test/distributed/org/apache/cassandra/distributed/test/SchemaTest.java Outdated

Copy link

Contributor

ekaterinadimitrova2 Sep 12, 2022 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: syncs

ekaterinadimitrova2 reviewed Sep 12, 2022

View reviewed changes

jacek-lewandowski added 3 commits September 13, 2022 08:18

Refactor and fix schema reset functionality

4ad2075

Patch by Jacek Lewandowski, reviewed by Andrés de la Peña and Ekaterina Dimitrova for CASSANDRA-17819

fix comments

f935aa8

DO NOT MERGE - CircleCI config

228bf5b

jacek-lewandowski force-pushed the CASSANDRA-17819-2 branch from a8842d6 to 228bf5b Compare September 13, 2022 06:45

jacek-lewandowski closed this Sep 16, 2022

jacek-lewandowski deleted the CASSANDRA-17819-2 branch September 16, 2022 08:05

-            } catch (RuntimeException ex) {
+            }
+            catch (RuntimeException ex)
+            {

	* As long as the certain version is advertised by some node, it is being tracked. As long as a version is tracked,
	* As long as a certain version is advertised by some node, it is being tracked. As long as a version is tracked,

	", waitQueue=" + waitQueue.getWaiting() +
	", waitQueueSize=" + waitQueue.getWaiting() +

	if (!clearCompletion.await(StorageService.SCHEMA_DELAY_MILLIS, TimeUnit.MILLISECONDS)) {
	if (!clearCompletion.await(StorageService.SCHEMA_DELAY_MILLIS, TimeUnit.MILLISECONDS))
	{

CASSANDRA-17819: Fix resetting the schema #1804

CASSANDRA-17819: Fix resetting the schema #1804

Uh oh!

Conversation

jacek-lewandowski commented Aug 22, 2022

Uh oh!

ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekaterinadimitrova2 Aug 30, 2022 •

edited

Loading

ekaterinadimitrova2 Aug 30, 2022 •

edited

Loading

ekaterinadimitrova2 Aug 30, 2022 •

edited

Loading

ekaterinadimitrova2 Aug 29, 2022 •

edited

Loading

ekaterinadimitrova2 Sep 12, 2022 •

edited

Loading