>From Michael Blow <[email protected]>: Michael Blow has submitted this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20471?usp=email )
Change subject: [NO ISSUE][*DB][STO] Fix IndexOutOfBoundsException removing last page... ...................................................................... [NO ISSUE][*DB][STO] Fix IndexOutOfBoundsException removing last page... ...when flushPtr is also that page (cherry picked from commit 75dd626ac1) Ext-ref:MB-68818 Change-Id: I1a710fa4773b2d75ff4f252399f4dadccdf3ed8d Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20471 Reviewed-by: Michael Blow <[email protected]> Reviewed-by: Ali Alsuliman <[email protected]> Tested-by: Michael Blow <[email protected]> --- M asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/GlobalVirtualBufferCacheTest.java M asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/GlobalVirtualBufferCache.java 2 files changed, 103 insertions(+), 3 deletions(-) Approvals: Michael Blow: Looks good to me, but someone else must approve; Verified Ali Alsuliman: Looks good to me, approved diff --git a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/GlobalVirtualBufferCacheTest.java b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/GlobalVirtualBufferCacheTest.java index b056bbe..0a1fba2 100644 --- a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/GlobalVirtualBufferCacheTest.java +++ b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/dataflow/GlobalVirtualBufferCacheTest.java @@ -19,6 +19,7 @@ package org.apache.asterix.test.dataflow; import java.io.File; +import java.lang.reflect.Field; import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collections; @@ -32,6 +33,7 @@ import org.apache.asterix.common.api.IDatasetLifecycleManager; import org.apache.asterix.common.config.DatasetConfig.DatasetType; import org.apache.asterix.common.config.StorageProperties.Option; +import org.apache.asterix.common.context.GlobalVirtualBufferCache; import org.apache.asterix.common.metadata.DataverseName; import org.apache.asterix.common.transactions.ITransactionContext; import org.apache.asterix.common.transactions.ITransactionManager; @@ -60,6 +62,7 @@ import org.apache.hyracks.storage.am.common.impls.AbstractTreeIndex; import org.apache.hyracks.storage.am.lsm.btree.impl.TestLsmBtree; import org.apache.hyracks.storage.am.lsm.common.api.ILSMDiskComponent; +import org.apache.hyracks.storage.am.lsm.common.api.ILSMIndex; import org.apache.hyracks.storage.am.lsm.common.impls.NoMergePolicyFactory; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -175,6 +178,103 @@ } } + @Test + public void testFlushPtrBoundsAfterRemovingLastIndex() throws Exception { + try { + // Create a scenario with multiple indexes to manipulate flushPtr + DataverseName dvName = DataverseName.createSinglePartName(StorageTestUtils.DATAVERSE_NAME); + + // Create 3 additional datasets to have multiple indexes + Dataset dataset2 = new TestDataset(dvName, "ds2", dvName, StorageTestUtils.DATA_TYPE_NAME, + StorageTestUtils.NODE_GROUP_NAME, NoMergePolicyFactory.NAME, null, + new InternalDatasetDetails(null, PartitioningStrategy.HASH, StorageTestUtils.PARTITIONING_KEYS, + null, null, null, false, null, null), + null, DatasetType.INTERNAL, StorageTestUtils.DATASET_ID + 2, 0); + + Dataset dataset3 = new TestDataset(dvName, "ds3", dvName, StorageTestUtils.DATA_TYPE_NAME, + StorageTestUtils.NODE_GROUP_NAME, NoMergePolicyFactory.NAME, null, + new InternalDatasetDetails(null, PartitioningStrategy.HASH, StorageTestUtils.PARTITIONING_KEYS, + null, null, null, false, null, null), + null, DatasetType.INTERNAL, StorageTestUtils.DATASET_ID + 3, 0); + + // Create indexes for first partition only for simplicity + PrimaryIndexInfo index2Info = StorageTestUtils.createPrimaryIndex(nc, dataset2, 0); + PrimaryIndexInfo index3Info = StorageTestUtils.createPrimaryIndex(nc, dataset3, 0); + + // Create index instances + IIndexDataflowHelperFactory factory2 = + new IndexDataflowHelperFactory(nc.getStorageManager(), index2Info.getFileSplitProvider()); + IIndexDataflowHelper helper2 = factory2.create(testCtxs[0].getJobletContext().getServiceContext(), 0); + helper2.open(); + TestLsmBtree index2 = (TestLsmBtree) helper2.getIndexInstance(); + helper2.close(); + + IIndexDataflowHelperFactory factory3 = + new IndexDataflowHelperFactory(nc.getStorageManager(), index3Info.getFileSplitProvider()); + IIndexDataflowHelper helper3 = factory3.create(testCtxs[0].getJobletContext().getServiceContext(), 0); + helper3.open(); + TestLsmBtree index3 = (TestLsmBtree) helper3.getIndexInstance(); + helper3.close(); + + // Access the global VBC through reflection to manipulate flushPtr + GlobalVirtualBufferCache globalVBC = (GlobalVirtualBufferCache) ncAppCtx.getVirtualBufferCache(); + + // Use reflection to access private fields + Field flushPtrField = GlobalVirtualBufferCache.class.getDeclaredField("flushPtr"); + flushPtrField.setAccessible(true); + + Field primaryIndexesField = GlobalVirtualBufferCache.class.getDeclaredField("primaryIndexes"); + primaryIndexesField.setAccessible(true); + + // Register memory components to add indexes to the list + // We already have primaryIndexes[0] and filteredPrimaryIndexes[0] registered + globalVBC.register(index2.getCurrentMemoryComponent()); + globalVBC.register(index3.getCurrentMemoryComponent()); + + // Get the current state + @SuppressWarnings("unchecked") + List<ILSMIndex> primaryIndexes = (List<ILSMIndex>) primaryIndexesField.get(globalVBC); + int currentSize = primaryIndexes.size(); + + // Set flushPtr to point to the last index (should be currentSize - 1) + flushPtrField.set(globalVBC, currentSize - 1); + int flushPtrBefore = (Integer) flushPtrField.get(globalVBC); + + // Verify we have the expected setup + Assert.assertTrue("Should have at least 3 indexes", currentSize >= 3); + Assert.assertEquals("flushPtr should point to last index", currentSize - 1, flushPtrBefore); + + // Remove the last index (the one flushPtr is pointing to) + ILSMIndex lastIndex = primaryIndexes.get(currentSize - 1); + globalVBC.unregister(lastIndex.getCurrentMemoryComponent()); + + // Check the state after removal + int newSize = primaryIndexes.size(); + int flushPtrAfter = (Integer) flushPtrField.get(globalVBC); + + // This demonstrates the bug: flushPtr should be within bounds [0, newSize) + // With the current buggy code (flushPtr > pos), flushPtr would be (currentSize-1-1) % newSize + // But if flushPtr == pos (pointing to removed index), it's not adjusted and points out of bounds + + Assert.assertEquals("List should be smaller after removal", currentSize - 1, newSize); + + // The bug manifests when flushPtr equals the position of the removed index + // In this case, flushPtr is not adjusted and points beyond the list + if (flushPtrBefore == currentSize - 1) { // We removed the index flushPtr was pointing to + Assert.assertTrue("flushPtr should be within bounds after removal", + flushPtrAfter >= 0 && flushPtrAfter < newSize); + } + + // Clean up + helper2.destroy(); + helper3.destroy(); + + } catch (Exception e) { + LOGGER.error("testFlushPtrBoundsAfterRemovingLastIndex failed", e); + Assert.fail(e.getMessage()); + } + } + private void initializeNc() throws Exception { List<Pair<IOption, Object>> opts = new ArrayList<>(); opts.add(Pair.of(Option.STORAGE_MEMORYCOMPONENT_GLOBALBUDGET, 128 * 1024L)); diff --git a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/GlobalVirtualBufferCache.java b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/GlobalVirtualBufferCache.java index f877143..12c660d 100644 --- a/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/GlobalVirtualBufferCache.java +++ b/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/GlobalVirtualBufferCache.java @@ -74,7 +74,7 @@ private final Set<ILSMIndex> flushingIndexes = Collections.synchronizedSet(new HashSet<>()); private final Set<ILSMMemoryComponent> flushingComponents = Collections.synchronizedSet(new HashSet<>()); - private volatile int flushPtr; + private int flushPtr; private final int filteredMemoryComponentMaxNumPages; private final int flushPageBudget; @@ -87,7 +87,7 @@ this.vbc = new VirtualBufferCache(allocator, storageProperties.getBufferCachePageSize(), (int) (storageProperties.getMemoryComponentGlobalBudget() / storageProperties.getMemoryComponentPageSize())); - this.flushPageBudget = (int) (storageProperties.getMemoryComponentGlobalBudget() + this.flushPageBudget = (int) ((double) storageProperties.getMemoryComponentGlobalBudget() / storageProperties.getMemoryComponentPageSize() * storageProperties.getMemoryComponentFlushThreshold()); this.filteredMemoryComponentMaxNumPages = storageProperties.getFilteredMemoryComponentMaxNumPages(); @@ -145,7 +145,7 @@ } if (primaryIndexes.isEmpty()) { flushPtr = 0; - } else if (flushPtr > pos) { + } else if (flushPtr >= pos) { // If the removed index is before flushPtr, we should decrement flushPtr by 1 so that // it still points to the same index. flushPtr = (flushPtr - 1) % primaryIndexes.size(); -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20471?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: asterixdb Gerrit-Branch: neo Gerrit-Change-Id: I1a710fa4773b2d75ff4f252399f4dadccdf3ed8d Gerrit-Change-Number: 20471 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Blow <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]>
