diff --git a/CHANGES.txt b/CHANGES.txt index 8598dfefbe..09d25aa676 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -42,6 +42,7 @@ Merged from 4.1: * Add Paxos v2 option and informatin in cassandra.yaml (CASSANDRA-21316) * Harden data resurrection startup check with atomic heartbeat file write with fallback (CASSANDRA-21290) Merged from 4.0: + * BTree.FastBuilder.reset() fails to clear savedBuffer and savedNextKey, causing ClassCastException and SSTable header corruption during schema disagreement (CASSANDRA-21216, CASSANDRA-21260) * Backport CASSANDRA-17810 fix and improve RTBoundValidator error messages (CASSANDRA-18282) * Rate limit password changes (CASSANDRA-21202) diff --git a/src/java/org/apache/cassandra/utils/btree/BTree.java b/src/java/org/apache/cassandra/utils/btree/BTree.java index 2d06c4db15..e198e06834 100644 --- a/src/java/org/apache/cassandra/utils/btree/BTree.java +++ b/src/java/org/apache/cassandra/utils/btree/BTree.java @@ -3286,7 +3286,7 @@ public class BTree * was constructed from for the contents of {@code buffer}. *
* For {@link FastBuilder} these are mostly the same, so they are fetched from a global cache and - * resized accordingly, but for {@link AbstractUpdater} we maintain a buffer of sizes. + * resized accordingly, but for {@link Updater} we maintain a buffer of sizes. */ int setDrainSizeMap(Object[] original, int keysInOriginal, Object[] branch, int keysInBranch) { @@ -3315,7 +3315,7 @@ public class BTree * was constructed from for the contents of {@code savedBuffer}. *
* For {@link FastBuilder} these are always the same size, so they are fetched from a global cache, - * but for {@link AbstractUpdater} we maintain a buffer of sizes. + * but for {@link Updater} we maintain a buffer of sizes. * * @return the size of {@code branch} */ @@ -3346,7 +3346,7 @@ public class BTree * was constructed from the contents of both {@code savedBuffer} and {@code buffer} *
* For {@link FastBuilder} these are mostly the same size, so they are fetched from a global cache - * and only the last items updated, but for {@link AbstractUpdater} we maintain a buffer of sizes. + * and only the last items updated, but for {@link Updater} we maintain a buffer of sizes. */ void setRedistributedSizeMap(Object[] branch, int steal) { @@ -3485,11 +3485,60 @@ public class BTree /** * Clear any references we might still retain, to avoid holding onto memory. - *
- * While this method is not strictly necessary, it exists to - * ensure the implementing classes are aware they must handle it. */ - abstract void reset(); + void reset() + { + leaf().count = 0; + clearLeafBuffer(leaf().buffer); + if (leaf().savedBuffer != null) + clearLeafBuffer(leaf().savedBuffer); + leaf().savedNextKey = null; + BranchBuilder branch = leaf().parent; + while (branch != null && branch.inUse) + { + branch.count = 0; + branch.hasRightChild = false; + clearBranchBuffer(branch.buffer); + if (branch.savedBuffer != null) + clearBranchBuffer(branch.savedBuffer); + branch.savedNextKey = null; + branch.clearSourceNode(); + branch.inUse = false; + branch = branch.parent; + } + Invariants.require(branch == null || (branch.count == 0 && !branch.hasRightChild)); + } + + /** + * Clear the contents of a leaf buffer, aborting once we encounter a null entry + * to save time on small trees + */ + private void clearLeafBuffer(Object[] array) + { + if (array[0] == null) + return; + // find first null entry; loop from beginning, to amortise cost over size of working set + int i = 1; + while (i < array.length && array[i] != null) + ++i; + Arrays.fill(array, 0, i, null); + } + + /** + * Clear the contents of a branch buffer, aborting once we encounter a null entry + * to save time on small trees + */ + private void clearBranchBuffer(Object[] array) + { + if (array[0] == null && array[MAX_KEYS] == null) + return; + // find first null entry; loop from beginning, to amortise cost over size of working set + int i = 1; + while (i < MAX_KEYS && array[i] != null) + ++i; + Arrays.fill(array, 0, i, null); + Arrays.fill(array, MAX_KEYS, MAX_KEYS + i + 1, null); + } } /** @@ -3544,14 +3593,7 @@ public class BTree @Override public void reset() { - Arrays.fill(leaf().buffer, null); - leaf().count = 0; - BranchBuilder branch = leaf().parent; - while (branch != null && branch.inUse) - { - branch.reset(); - branch = branch.parent; - } + super.reset(); } public boolean validateEmpty() @@ -3580,62 +3622,6 @@ public class BTree } } - private static abstract class AbstractUpdater extends AbstractFastBuilder implements AutoCloseable - { - void reset() - { - assert leaf().count == 0; - clearLeafBuffer(leaf().buffer); - if (leaf().savedBuffer != null) - Arrays.fill(leaf().savedBuffer, null); - - BranchBuilder branch = leaf().parent; - while (branch != null && branch.inUse) - { - branch.count = 0; - branch.hasRightChild = false; - clearBranchBuffer(branch.buffer); - if (branch.savedBuffer != null && branch.savedBuffer[0] != null) - Arrays.fill(branch.savedBuffer, null); // by definition full, if non-empty - branch.inUse = false; - branch = branch.parent; - } - Invariants.require(branch == null || (branch.count == 0 && !branch.hasRightChild)); - } - - /** - * Clear the contents of a branch buffer, aborting once we encounter a null entry - * to save time on small trees - */ - private void clearLeafBuffer(Object[] array) - { - if (array[0] == null) - return; - // find first null entry; loop from beginning, to amortise cost over size of working set - int i = 1; - while (i < array.length && array[i] != null) - ++i; - Arrays.fill(array, 0, i, null); - } - - /** - * Clear the contents of a branch buffer, aborting once we encounter a null entry - * to save time on small trees - */ - private void clearBranchBuffer(Object[] array) - { - if (array[0] == null) - return; - - // find first null entry; loop from beginning, to amortise cost over size of working set - int i = 1; - while (i < MAX_KEYS && array[i] != null) - ++i; - Arrays.fill(array, 0, i, null); - Arrays.fill(array, MAX_KEYS, MAX_KEYS + i + 1, null); - } - } - /** * A pooled object for modifying an existing tree with a new (typically smaller) tree. *
@@ -3650,7 +3636,7 @@ public class BTree
* Searches within both trees to accelerate the process of modification, instead of performing a simple
* iteration over the new tree.
*/
- static class Updater
* The approach taken here hopefully balances simplicity, garbage generation and execution time.
*/
- static abstract class AbstractSeekingTransformer extends AbstractUpdater implements AutoCloseable
+ static abstract class AbstractSeekingTransformer extends AbstractFastBuilder implements AutoCloseable
{
/**
* An iterator over the tree we are updating
diff --git a/test/unit/org/apache/cassandra/utils/btree/BTreeTest.java b/test/unit/org/apache/cassandra/utils/btree/BTreeTest.java
index ef34677621..d51322f2f6 100644
--- a/test/unit/org/apache/cassandra/utils/btree/BTreeTest.java
+++ b/test/unit/org/apache/cassandra/utils/btree/BTreeTest.java
@@ -428,6 +428,27 @@ public class BTreeTest
assertEquals(count, i);
}
+ @Test
+ public void testFastBuilderResetClearsSavedState()
+ {
+ // Add >31 items to trigger overflow (savedBuffer/savedNextKey population)
+ try (BTree.FastBuilderUpdateFunction that count the number of call made to apply for each value.
*/