CASSANDRA-20493 Paxos infinite loop in mixed mode #4015
Conversation
test/distributed/org/apache/cassandra/distributed/upgrade/MixedModePaxosTest.java
Outdated
Show resolved
Hide resolved
| { | ||
| String keyspace = KEYSPACE; | ||
| String table = "tbl"; | ||
| int gcGrace = 10; // 1 day |
| .withConfig(c -> c.with(Feature.GOSSIP, Feature.NETWORK)) | ||
| .nodes(2) | ||
| .nodesToUpgrade(1) | ||
| // all upgrades from v30 up, excluding v30->v3X and from v40 |
There was a problem hiding this comment.
This also seems like maybe a C&P comment that doesn't apply since it mentions additional upgrades?
| * less than its own and will update them with its slightly higher zero ballot and empty partition update. In cases | ||
| * where this is the first paxos operation on a key, or the previously ttl'd paxos data on disk had been purged, this | ||
| * would just add a retry step as it updated the 4.0 and lower hosts with it's zero ballot. On nodes where there was | ||
| * ttl'd paxos data though, the ttl'd data on disk would shadow this update and the conflicting 'zero' value would |
There was a problem hiding this comment.
Maybe just extend this comment that the TTLed data would have a timestamp > 0 which shadows none even if the TTLed data doesn't end up returned in a read. It should be obvious, but you have to make the connection between the timestamp for the paxos system table row being the timestamp of the a ballot which those less familiar with the table won't think of immediately.
| @@ -38,6 +38,7 @@ | |||
| public class Ballot extends TimeUUID | |||
| { | |||
| public static final long serialVersionUID = 1L; | |||
| private static final long LEGACY_MIN_CLOCK_SEQ_AND_NODE = 0x8080808080808080L; | |||
There was a problem hiding this comment.
Do you think it'd be unreasonable to make MIN_CLOCK_SEQ_AND_NODE public on TimeUUID? If so, maybe adding a small comment about the source of this constant could be useful for context.
There was a problem hiding this comment.
no I think that would be fine. I'd thought it had been removed from TimeUUID from some reason, which is why I copied it here
| * Tests the mixed mode loop bug in CASSANDRA-20493 | ||
| * | ||
| * Paxos uses a 'zero' ballot in place of null when it doesn't find a ballot in system.paxos. CEP-14 changed the lsb | ||
| * of the zero ballot uuid from -9187201950435737472 to 0. It also removed the check added in CASSANDRA-12043, since |
There was a problem hiding this comment.
It could be useful to use hexadecimal 0x8080808080808080 in place of integer constant here.
There was a problem hiding this comment.
The integer constant makes it clear that the old lsb will be interpreted as less than 0. However, I reworded this part to indicate it came from TimeUUID.MIN_CLOCK_SEQ_AND_NODE and include the hex value
| * and empty partition update. | ||
| * | ||
| * In cases where this is the first paxos operation on a key, or the previously ttl'd paxos data on disk had been purged, | ||
| * this would just add a retry step as it updated the 4.0 and lower hosts with it's zero ballot. |
|
I don't think we intended to change the definition of the zero ballot in CEP-14, so might be worth double checking how that came to happen, and whether it's better to roll it back (maybe not for systems already using v2). |
I’m 99.9% sure this was not intentional. I’d thought about this, and if there weren’t already clusters with the CEP-14 changes then rolling this back would be the right thing to do. The problem with rolling this back at this point is that existing 4.1 clusters have had the check from CASSANDRA-12043 removed so 4.1 coordinators are pretty sensitive to Ballot#none changes, whereas 4.0 and lower clusters disregard any MRC that would have been ttl’d so this isn't an issue for them. |
Patch by Blake Eggleston, reviewed by Alex Petrov and Ariel Weisberg for CASSANDRA-20493
bdeggleston
force-pushed
the
C20493-mixed-mode-paxos-loop
branch
from
789be26 to
9c051a4
Compare
No description provided.