From 45937def313bbb32024ae890f830e23bcc6ccae5 Mon Sep 17 00:00:00 2001 From: Aleksey Yeshchenko Date: Tue, 18 Sep 2018 13:12:11 +0100 Subject: [PATCH] DESC order reads can fail to return the last Unfiltered in the partition patch by Aleksey Yeschenko; reviewed by Sam Tunnicliffe and Benedict Elliott Smith for CASSANDRA-14766 --- CHANGES.txt | 1 + .../cassandra/db/UnfilteredDeserializer.java | 117 ++++++++++++------ ...es-legacy_ka_14766-ka-1-CompressionInfo.db | Bin 0 -> 43 bytes ...legacy_tables-legacy_ka_14766-ka-1-Data.db | Bin 0 -> 103 bytes ...cy_tables-legacy_ka_14766-ka-1-Digest.sha1 | 1 + ...gacy_tables-legacy_ka_14766-ka-1-Filter.db | Bin 0 -> 16 bytes ...egacy_tables-legacy_ka_14766-ka-1-Index.db | Bin 0 -> 134 bytes ..._tables-legacy_ka_14766-ka-1-Statistics.db | Bin 0 -> 4450 bytes ...acy_tables-legacy_ka_14766-ka-1-Summary.db | Bin 0 -> 92 bytes ...legacy_tables-legacy_ka_14766-ka-1-TOC.txt | 8 ++ .../io/sstable/LegacySSTableTest.java | 27 +++- 11 files changed, 113 insertions(+), 41 deletions(-) create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-CompressionInfo.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Data.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Digest.sha1 create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Filter.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Index.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Statistics.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Summary.db create mode 100644 test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-TOC.txt diff --git a/CHANGES.txt b/CHANGES.txt index 195c97c93a..43628b258a 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.18 + * DESC order reads can fail to return the last Unfiltered in the partition (CASSANDRA-14766) * Fix corrupted collection deletions for dropped columns in 3.0 <-> 2.{1,2} messages (CASSANDRA-14568) * Fix corrupted static collection deletions in 3.0 <-> 2.{1,2} messages (CASSANDRA-14568) * Handle failures in parallelAllSSTableOperation (cleanup/upgradesstables/etc) (CASSANDRA-14657) diff --git a/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java b/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java index 0aa574151c..62ad76a1b3 100644 --- a/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java +++ b/src/java/org/apache/cassandra/db/UnfilteredDeserializer.java @@ -245,10 +245,14 @@ public abstract class UnfilteredDeserializer // The next Unfiltered to return, computed by hasNext() private Unfiltered next; - // A temporary storage for an unfiltered that isn't returned next but should be looked at just afterwards - private Unfiltered saved; - private boolean isFirst = true; + // Saved position in the input after the next Unfiltered that will be consumed + private long nextConsumedPosition; + + // A temporary storage for an Unfiltered that isn't returned next but should be looked at just afterwards + private Stash stash; + + private boolean couldBeStartOfPartition = true; // The Unfiltered as read from the old format input private final UnfilteredIterator iterator; @@ -258,7 +262,15 @@ public abstract class UnfilteredDeserializer // Tracks the size of the last LegacyAtom read from disk, because this needs to be accounted // for when marking lastConsumedPosition after readNext/skipNext - private long bytesReadForNextAtom; + // Reading/skipping an Unfiltered consumes LegacyAtoms from the underlying legacy atom iterator + // e.g. hasNext() -> iterator.hasNext() -> iterator.readRow() -> atoms.next() + // The stop condition of the loop which groups legacy atoms into rows causes that AtomIterator + // to read in the first atom which doesn't belong in the row. So by that point, our position + // is actually past the end of the next Unfiltered. To compensate, we record the size of + // the last LegacyAtom read and subtract it from the current position when we calculate lastConsumedPosition. + // If we don't, then when reading an indexed block, we can over correct and may think that we've + // exhausted the block before we actually have. + private long bytesReadForNextAtom = 0L; private OldFormatDeserializer(CFMetaData metadata, DataInputPlus in, @@ -313,27 +325,55 @@ public abstract class UnfilteredDeserializer { while (next == null) { - if (saved == null && !iterator.hasNext()) - return false; - - next = saved == null ? iterator.next() : saved; - saved = null; - - // The sstable iterators assume that if there is one, the static row is the first thing this deserializer will return. - // However, in the old format, a range tombstone with an empty start would sort before any static cell. So we should - // detect that case and return the static parts first if necessary. - if (isFirst && iterator.hasNext() && isStatic(iterator.peek())) + if (null != stash) { - saved = next; - next = iterator.next(); + next = stash.unfiltered; + nextConsumedPosition = stash.consumedPosition; + stash = null; } - isFirst = false; + else + { + if (!iterator.hasNext()) + return false; + next = iterator.next(); + nextConsumedPosition = currentPosition() - bytesReadForNextAtom; + } + + /* + * The sstable iterators assume that if there is one, the static row is the first thing this deserializer will return. + * However, in the old format, a range tombstone with an empty start would sort before any static cell. So we should + * detect that case and return the static parts first if necessary. + */ + if (couldBeStartOfPartition && next.isRangeTombstoneMarker() && next.clustering().size() == 0 && iterator.hasNext()) + { + Unfiltered unfiltered = iterator.next(); + long consumedPosition = currentPosition() - bytesReadForNextAtom; + + stash = new Stash(unfiltered, consumedPosition); + + /* + * reorder next and stash (see the comment above that explains why), but retain their positions + * it's ok to do so since consumedPosition value is only used to determine if we have gone past + * the end of the index ‘block’; since the edge case requires that the first value be the ‘bottom’ + * RT bound (i.e. with no byte buffers), this has a small and well-defined size, and it must be + * the case that both unfiltered are in the same index ‘block’ if we began at the beginning of it. + * if we don't do this, however, we risk aborting early and not returning the BOTTOM rt bound, + * if the static row is large enough to cross block boundaries. + */ + if (isStatic(unfiltered)) + { + stash.unfiltered = next; + next = unfiltered; + } + } + couldBeStartOfPartition = false; // When reading old tables, we sometimes want to skip static data (due to how staticly defined column of compact // tables are handled). if (skipStatic && isStatic(next)) next = null; } + return true; } catch (IOError e) @@ -376,18 +416,17 @@ public abstract class UnfilteredDeserializer throw new IllegalStateException(); Unfiltered toReturn = next; next = null; - lastConsumedPosition = currentPosition() - bytesReadForNextAtom(); + lastConsumedPosition = nextConsumedPosition; return toReturn; } public void skipNext() throws IOException { - if (!hasNext()) - throw new UnsupportedOperationException(); - next = null; - lastConsumedPosition = currentPosition() - bytesReadForNextAtom(); + readNext(); } + // in case we had to reorder an empty RT bound with a static row, this won't be returning the precise unconsumed size, + // that corresponds to the last returned Unfiltered, but use the natural order in the sstable instead public long bytesReadForUnconsumedData() { if (!(in instanceof FileDataInput)) @@ -396,28 +435,26 @@ public abstract class UnfilteredDeserializer return currentPosition() - lastConsumedPosition; } - // Reading/skipping an Unfiltered consumes LegacyAtoms from the underlying legacy atom iterator - // e.g. hasNext() -> iterator.hasNext() -> iterator.readRow() -> atoms.next() - // The stop condition of the loop which groups legacy atoms into rows causes that AtomIterator - // to read in the first atom which doesn't belong in the row. So by that point, our position - // is actually past the end of the next Unfiltered. To compensate, we record the size of - // the last LegacyAtom read and subtract it from the current position when we calculate lastConsumedPosition. - // If we don't, then when reading an indexed block, we can over correct and may think that we've - // exhausted the block before we actually have. - private long bytesReadForNextAtom() - { - // If we've read anything at all then we will have recorded this in bytesReadForNextAtom, - // but being extra careful here just incase this method is called before any reads happen. - return iterator.atoms.next == null ? 0 : bytesReadForNextAtom; - } - public void clearState() { next = null; - saved = null; + stash = null; + couldBeStartOfPartition = true; iterator.clearState(); lastConsumedPosition = currentPosition(); - bytesReadForNextAtom = 0; + bytesReadForNextAtom = 0L; + } + + private static final class Stash + { + private Unfiltered unfiltered; + long consumedPosition; + + private Stash(Unfiltered unfiltered, long consumedPosition) + { + this.unfiltered = unfiltered; + this.consumedPosition = consumedPosition; + } } // Groups atoms from the input into proper Unfiltered. @@ -543,7 +580,7 @@ public abstract class UnfilteredDeserializer // Wraps the input of the deserializer to provide an iterator (and skip shadowed atoms). // Note: this could use guava AbstractIterator except that we want to be able to clear // the internal state of the iterator so it's cleaner to do it ourselves. - private class AtomIterator implements PeekingIterator + private static class AtomIterator implements PeekingIterator { private final Supplier atomReader; private boolean isDone; diff --git a/test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-CompressionInfo.db b/test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-CompressionInfo.db new file mode 100644 index 0000000000000000000000000000000000000000..b5b5246f1151aab706df379bee12b490a6f1021a GIT binary patch literal 43 gcmZSJ^@%cZ&d)6Z{TELU}6yB7hz~*U|=mv%qdOfWLUqT%xuYO1_s7228;~< zA2JIu2{3Xpu?sQTd#*1MV*3A9nhD5`$`S{&fztK`_dZ$OAKG4L}$)v%T&=9H!~Km`~X7+?Yr)j|-8MFpyW384UD1T#Va KtdzwJrUL-#Ne%4) literal 0 HcmV?d00001 diff --git a/test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Statistics.db b/test/data/legacy-sstables/ka/legacy_tables/legacy_ka_14766/legacy_tables-legacy_ka_14766-ka-1-Statistics.db new file mode 100644 index 0000000000000000000000000000000000000000..d4b0526c3296f99b6a2dcb633d0af2129c67c96d GIT binary patch literal 4450 zcmeI$`AZaG7zgk73&XshdEVK1-+AAed039) zM6Rz4lXkqcrAnJ1ZCPAIjwM59%r&NGnsw>MygXyJ$zs%*GOfCp4OVla#bmaaO-V+J zHOrcnlWn%>iwa-FoeA71o$M-Vr_XS|4U2IEbP%Nf@( zwxM}uKKLn2+{(BKEevM74J|UiFIHk);aZTSC?g4^s%i$>OCGO#U3ve+xNi$u?Lhb6 zfz}>D4_b;2ZbuJ~K#w_$)}^7N529nPppyooR|TQ-7NCob=qhIYwiqz(2nF+@AJJ(G z(FbvT@{UIk7gF$f7H*@(wENIrp}_U8cyJfu%9|x%)rV@ZcZw72(_;qr>j?mBOt>EX z4X+^{*f0n@q$3y{VoL^(YQ^~u@5cF#EV~NrXLP;<&z+6^TM}%C_}X_`@Ycjy@PY4B z!S&bi{yeXT`^ozb_(I$B`nX--EhA5X?_8TmEsVVc?^j`;<{^!X!3D5B#D*0Oc5xPU{tv${>XIAPanv;_ z2S!rgt0*d^{(7N3mY5&sx4MBkea;;E{PWdGm+14)JNIhn^Dmg~9hb@ZLg(q3g~Xzj zx9=fVjBeE?PeABr8vQ9Qp_s%cq&mWAJ`ff8wpl~sdfThU z^~CclYfsbuEOShYqU$+3*-%wR+83>^Z|NjH^z8YSD&nSN$7^YS?bpKFqyNNt?e@FB zVu%yge)+lWkGVW|9pAx!=U@Uk$5oYcF8MqK6IU{ZcY+l2