CASSANDRA-20449 Serialization can lose complex deletions in a mutation with multiple collections in a row #3987
Conversation
| COLUMN_COMPARATOR, isStatic() ? FIRST_COMPLEX_STATIC : FIRST_COMPLEX_REGULAR, 0L); | ||
| return result == Cell.MAX_DELETION_TIME; | ||
| return result == Long.MAX_VALUE; |
There was a problem hiding this comment.
I simply reverted here rather than trying to do something less brittle around the stop value. Open to doing that if we think it really helps...
There was a problem hiding this comment.
So when working on 14227 the whole code was littered with magical sentinel values and it was very difficult to make sense of things. I did my best to provide semantic meaning to those with proper constant names and apparently broke this one. Could you name this sthg meaningful?
| throw new IllegalStateException("Object " + o + " maps to a buffer that already exists in the set"); | ||
| // decompose/serializer doesn't use the isMultiCell, so safe to do this | ||
| return SetType.getInstance(BytesType.instance, false).decompose(bbs); | ||
| } |
There was a problem hiding this comment.
This is part of a larger set of capabilities already in trunk.
| // If the complex deletion is not properly serialized, node 2 will think the update on s2 was an append... | ||
| assertRows(CLUSTER.get(2).executeInternal(select), row(0, 0, set(1, 2), set(2), set(1, 2))); | ||
| } | ||
| } |
There was a problem hiding this comment.
This probably needs to be accompanied by proper fuzz testing of the Mutation serialization complex in trunk, where we have better tooling for that.
CC @dcapwell
There was a problem hiding this comment.
We should be able to assert roundtrip serialization of Mutations (and add Mutation.equals). I was also thinking we'd be able to find this in one of David's tests that uses a model and runs commands like unsafe bounce (where commit log replay occurs on startup). But we'd have to read across all replicas at NODE_LOCAL to find this, since including a replica with the correct update in the memtable would have hidden the issue.
TopologyMixupTestBase appears to do an unsafe shutdown but HarryTopologyMixupTest doesn't appear to generate mixed set overwrite + set append operations, so we wouldn't find it there as it's currently implemented.
maedhroz
force-pushed
the
CASSANDRA-20449-5.0
branch
from
9299e6e to
ac9607b
Compare
| { | ||
| Set<?> set = (Set<?>) obj; | ||
| // convert subtypes to BB | ||
| Set<ByteBuffer> bbs = new LinkedHashSet<>(); |
There was a problem hiding this comment.
Can set initial capacity here to match obj?
There was a problem hiding this comment.
I wanted to make this match what was in trunk...although I agree in principle.
| // If the complex deletion is not properly serialized, node 2 will think the update on s2 was an append... | ||
| assertRows(CLUSTER.get(2).executeInternal(select), row(0, 0, set(1, 2), set(2), set(1, 2))); | ||
| } | ||
| } |
There was a problem hiding this comment.
We should be able to assert roundtrip serialization of Mutations (and add Mutation.equals). I was also thinking we'd be able to find this in one of David's tests that uses a model and runs commands like unsafe bounce (where commit log replay occurs on startup). But we'd have to read across all replicas at NODE_LOCAL to find this, since including a replica with the correct update in the memtable would have hidden the issue.
TopologyMixupTestBase appears to do an unsafe shutdown but HarryTopologyMixupTest doesn't appear to generate mixed set overwrite + set append operations, so we wouldn't find it there as it's currently implemented.
|
|
||
| // If the complex deletion is not properly serialized, node 2 will think the update on s2 was an append... | ||
| assertRows(CLUSTER.get(2).executeInternal(select), row(0, 0, set(1, 2), set(2), set(1, 2))); | ||
| } |
There was a problem hiding this comment.
Confirmed test fails without the changes, passes with 👍
| @@ -1823,7 +1824,7 @@ private static <V> int find(Object[] btree, V from, Comparator<V> comparator) | |||
|
|
|||
| private static boolean isStopSentinel(long v) | |||
| { | |||
| return v == Long.MAX_VALUE; | |||
| return v == STOP_SENTINEL_VALUE; | |||
There was a problem hiding this comment.
Could you update docstring on BTree#accumulate? Also looks like we stopped using the sentinel Long.MIN_VALUE here a while ago, seems like a mistake in the comment.
Not something that should be addressed in this patch, but feels like we should have a separate AbortableLongAccumulator that signals early termination out of band to make this even safer.
maedhroz
force-pushed
the
CASSANDRA-20449-5.0
branch
from
ac9607b to
b856fd4
Compare
…collections in a row patch by Caleb Rackliffe; reviewed by Berenguer Blasi and Abe Ratnofsky for CASSANDRA-20449
maedhroz
force-pushed
the
CASSANDRA-20449-5.0
branch
from
b856fd4 to
11448dd
Compare
|
+1 |
patch by Caleb Rackliffe; reviewed by ? for CASSANDRA-20449