-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Non-serial single partition reads on Accord #3568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
196cf20 to
1f27091
Compare
dcapwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this all makes sense, but would love to see more testing on this...
I ran this against harry and it was looking fine at least... ill come back to this Monday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| checkState(tms != null || fromMode == TransactionalMigrationFromMode.none); | |
| checkState(tms != null || fromMode == TransactionalMigrationFromMode.none, "%s == null or %s != none", tms, fromMode); |
if this happens, good to know why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true? If you enable migration and this is the first time its touched, does a TableMigrationState exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether it's true is an artifact of how modifying the migration mode works and yes it's true. If you alter a table to start migration a TableMigrationState is created.
Before you could have a table that the table params said was migrating, but there would be no table migration state and it was more unwieldy to deal with because you always had to remember to check both the table parameters and the table migration state to know what was going on.
src/java/org/apache/cassandra/service/consensus/TransactionalMode.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SELECT * FROM ... WHERE pk IN (?, ?);
I would expect the rows to be in token order, so do think this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't one query it's a list of queries which each return a separate iterator and are coordinated as separate reads in Cassandra. This list contains queries that can be on separate partitions and it can come from SelectStatement and ModificationStatement (which can be BatchStatement).
I don't see anything that ensures the list of queries is in token order or that that is even the correct order especially in the case of BatchStatement where the reads are for read before write.
I think for at least the batch case the queries are in statement order and that needs to be preserved so I am now 95% confident it is the opposite and they need to be in the original order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker for this patch, but we should find testing that hits this case and make sure we maintain the existing behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what case is this? if the read/writes from are accord... then are we migrating from full to full?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just 100% wrong, and I'll also bet that if this thing actually worked it would have made a bunch of the tests fail when we test live migration to full.
We don't have the looping and splitting to handle races and route different parts of the query to Accord/Cassandra so really no migration should be allowed.
I think for today we should just remove this check.
|
another thing to think about, we block things that are not safe in transactions but this logic isn't being applied to mutations nor these selects... so what is the plan for blocking non-safe transactional things? |
|
From our conversations about things that aren't being blocked.
You are working on blocking TTLs, I think for the other 3 I will file JIRAs. |
30c7eaf to
0685729
Compare
0685729 to
b279ad1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove dead code?
8ac4f52 to
29abae1
Compare
72acfb6 to
7debd8b
Compare
d3b3df5 to
f3a4f34
Compare
2185ed2 to
676ee51
Compare
a775990 to
992dbba
Compare
Patch by Ariel Weisberg; Reviewed by Benedict Elliott Smith for CASSANDRA-19951
992dbba to
4b69672
Compare
No description provided.