>From Murtadha Hubail <[email protected]>: Murtadha Hubail has uploaded this change for review. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21656?usp=email )
Change subject: [NO ISSUE][STO] Drop resource state of released partitions ...................................................................... [NO ISSUE][STO] Drop resource state of released partitions - user model changes: no - storage format changes: no - interface changes: no Details: - A node keeps its resource-level view of a storage partition after it stops owning it. Nothing invalidates that view on release: the only callers of invalidateResource/clearResourcesCache are the replication tasks, the node's own delete, and per-index cleanup. - So while another node owns the partition, a resource deleted there is never forgotten here, and re-acquiring the partition resumes from what this node remembered rather than from the shared storage's truth. - Creating a resource at that path then fails: IndexBuilder#build sees a leftover through the repository's cached view, and the delete that clears it re-reads the metadata file, finds nothing, and throws HYR0055. The create is rolled back cleanly but fails, and the error reaches the client unmapped. - Fix the root cause by invalidating the partition's resources when it is released, and make the create path's best-effort pre-delete tolerate a leftover that is already gone, the same tolerance IndexDropOperatorNodePushable has under IF_EXISTS. Tests: - Added PersistentLocalResourceRepositoryTest# invalidateReleasedPartitionResources: with the resource's metadata file deleted behind the node's back, releasing the partition must drop the cached view. - Added IndexBuilderTest: a create whose leftover resource is already gone succeeds; any other delete failure still fails the create. Ext-ref: MB-73587 Co-Authored-By: Claude Opus 5 <[email protected]> Change-Id: Ieede4752d680832e7801929d26744720a3c05244 --- M asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/ReplicaManager.java M asterixdb/asterix-app/src/test/java/org/apache/asterix/test/storage/PersistentLocalResourceRepositoryTest.java M asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java M hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java A hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/build/IndexBuilderTest.java 5 files changed, 197 insertions(+), 1 deletion(-) git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/56/21656/1 diff --git a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/ReplicaManager.java b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/ReplicaManager.java index a1eee08..c6accd9 100644 --- a/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/ReplicaManager.java +++ b/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/ReplicaManager.java @@ -38,6 +38,7 @@ import org.apache.hyracks.api.exceptions.HyracksDataException; import org.apache.hyracks.control.nc.NodeControllerService; import org.apache.hyracks.storage.common.LocalResource; +import org.apache.hyracks.util.annotations.AiProvenance; import org.apache.hyracks.util.annotations.ThreadSafe; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -139,6 +140,7 @@ } @Override + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_CLI, contributionKind = AiProvenance.ContributionKind.ASSISTED) public synchronized void release(int partition) throws HyracksDataException { if (!partitions.containsKey(partition)) { return; @@ -148,6 +150,8 @@ for (IPartitionReplica replica : partitionReplicas) { appCtx.getReplicationManager().unregister(replica); } + ((PersistentLocalResourceRepository) appCtx.getLocalResourceRepository()) + .invalidatePartitionResources(partition); partitions.remove(partition); } diff --git a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/storage/PersistentLocalResourceRepositoryTest.java b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/storage/PersistentLocalResourceRepositoryTest.java index 59efef4..1f4eb1f 100644 --- a/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/storage/PersistentLocalResourceRepositoryTest.java +++ b/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/storage/PersistentLocalResourceRepositoryTest.java @@ -39,6 +39,7 @@ import org.apache.hyracks.api.io.FileReference; import org.apache.hyracks.storage.am.lsm.common.impls.AbstractLSMIndexFileManager; import org.apache.hyracks.storage.common.LocalResource; +import org.apache.hyracks.util.annotations.AiProvenance; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -224,6 +225,32 @@ } } + @Test + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_CLI, contributionKind = AiProvenance.ContributionKind.TEST_GENERATED, notes = "Regression test for stale resource state of a released storage partition (MB-73587)") + public void invalidateReleasedPartitionResources() throws Exception { + final INcApplicationContext ncAppCtx = (INcApplicationContext) integrationUtil.ncs[0].getApplicationContext(); + final String nodeId = ncAppCtx.getServiceContext().getNodeId(); + final String datasetName = "ds"; + TestDataUtil.createIdOnlyDataset(datasetName); + final Dataset dataset = TestDataUtil.getDataset(integrationUtil, datasetName); + final String indexPath = TestDataUtil.getIndexPath(integrationUtil, dataset, nodeId); + PersistentLocalResourceRepository localResourceRepository = + (PersistentLocalResourceRepository) ncAppCtx.getLocalResourceRepository(); + final LocalResource resource = localResourceRepository.get(indexPath); + Assert.assertNotNull(resource); + final int partition = ((DatasetLocalResource) resource.getResource()).getPartition(); + // simulate the resource being deleted while this node does not own its storage partition: the partition's + // new owner performs the delete, so the metadata file is gone but this node is never told about it + final FileReference indexDirRef = ncAppCtx.getIoManager().resolve(indexPath); + final File indexMetadataFile = new File(indexDirRef.getFile(), StorageConstants.METADATA_FILE_NAME); + Assert.assertTrue(indexMetadataFile.exists()); + Files.delete(indexMetadataFile.toPath()); + // releasing the partition must drop this node's view of its resources; otherwise creating a resource at the + // same path after the partition is re-acquired finds a leftover that no longer exists + localResourceRepository.invalidatePartitionResources(partition); + Assert.assertNull(localResourceRepository.get(indexPath)); + } + private void ensureInvalidComponentDeleted(String indexDir, String componentSeq, PersistentLocalResourceRepository localResourceRepository, DatasetLocalResource lr) throws IOException { Path btreePath = Paths.get(indexDir, diff --git a/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java b/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java index bfce93c..40daa7b 100644 --- a/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java +++ b/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java @@ -75,6 +75,7 @@ import org.apache.hyracks.storage.common.ILocalResourceRepository; import org.apache.hyracks.storage.common.LocalResource; import org.apache.hyracks.util.ExitUtil; +import org.apache.hyracks.util.annotations.AiProvenance; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -354,6 +355,24 @@ resourceCache.invalidate(relativePath); } + /** + * Invalidates any cached view of the resources of storage partition {@code partition}. Must be called when the + * node stops owning the partition. + */ + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_CLI, contributionKind = AiProvenance.ContributionKind.GENERATED) + public void invalidatePartitionResources(int partition) { + // write access excludes get(), which populates the cache from the resource's metadata file + beforeWriteAccess(); + try { + resourceCache.asMap().entrySet().removeIf(entry -> { + DatasetLocalResource dsResource = (DatasetLocalResource) entry.getValue().getResource(); + return dsResource.getPartition() == partition; + }); + } finally { + afterWriteAccess(); + } + } + public void clearResourcesCache() { resourceCache.invalidateAll(); } diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java b/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java index 7109aab..25bf2da 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/build/IndexBuilder.java @@ -18,6 +18,8 @@ */ package org.apache.hyracks.storage.am.common.build; +import static org.apache.hyracks.api.exceptions.ErrorCode.RESOURCE_DOES_NOT_EXIST; + import java.io.IOException; import org.apache.hyracks.api.application.INCServiceContext; @@ -33,6 +35,7 @@ import org.apache.hyracks.storage.common.IStorageManager; import org.apache.hyracks.storage.common.LocalResource; import org.apache.hyracks.storage.common.file.IResourceIdFactory; +import org.apache.hyracks.util.annotations.AiProvenance; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -86,7 +89,7 @@ LocalResource lr = localResourceRepository.get(resourceRelPath); long resourceId = lr == null ? -1 : lr.getId(); if (resourceId != -1) { - localResourceRepository.delete(resourceRelPath); + deleteLeftoverResource(localResourceRepository); } resourceId = resourceIdFactory.createId(); IResource resource = localResourceFactory.createResource(resourceRef); @@ -124,4 +127,23 @@ } lcManager.register(resourceRelPath, index); } + + /** + * Clears a leftover resource at {@link #resourceRelPath} so that the index about to be created starts from a + * clean slate. This is best-effort cleanup: the repository may report the resource as present from a cached + * view while it is already gone from the underlying storage (e.g. it was deleted by another node while this + * node did not own its storage partition), in which case there is nothing to clear and the create should + * proceed. + */ + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_CLI, contributionKind = AiProvenance.ContributionKind.GENERATED) + private void deleteLeftoverResource(ILocalResourceRepository localResourceRepository) throws HyracksDataException { + try { + localResourceRepository.delete(resourceRelPath); + } catch (HyracksDataException e) { + if (!e.matches(RESOURCE_DOES_NOT_EXIST)) { + throw e; + } + LOGGER.warn("Leftover resource {} is already gone on index create", resourceRelPath); + } + } } diff --git a/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/build/IndexBuilderTest.java b/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/build/IndexBuilderTest.java new file mode 100644 index 0000000..cb5fdcf --- /dev/null +++ b/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/test/java/org/apache/hyracks/storage/am/common/build/IndexBuilderTest.java @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.hyracks.storage.am.common.build; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.File; + +import org.apache.hyracks.api.application.INCServiceContext; +import org.apache.hyracks.api.exceptions.ErrorCode; +import org.apache.hyracks.api.exceptions.HyracksDataException; +import org.apache.hyracks.api.io.FileReference; +import org.apache.hyracks.api.io.IIOManager; +import org.apache.hyracks.storage.common.IIndex; +import org.apache.hyracks.storage.common.ILocalResourceRepository; +import org.apache.hyracks.storage.common.IResource; +import org.apache.hyracks.storage.common.IResourceFactory; +import org.apache.hyracks.storage.common.IResourceLifecycleManager; +import org.apache.hyracks.storage.common.IStorageManager; +import org.apache.hyracks.storage.common.LocalResource; +import org.apache.hyracks.storage.common.file.IResourceIdFactory; +import org.apache.hyracks.util.annotations.AiProvenance; +import org.junit.Assert; +import org.junit.Test; + +/** + * Tests how {@link IndexBuilder} clears a leftover resource before creating an index. + */ +@AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_CLI, contributionKind = AiProvenance.ContributionKind.TEST_GENERATED, notes = "Regression test for the create path's intolerant pre-delete (MB-73587)") +public class IndexBuilderTest { + + private static final String RESOURCE_PATH = "storage/partition_5/Default/Default/airline/0/airline"; + private static final long LEFTOVER_RESOURCE_ID = 693; + private static final long NEW_RESOURCE_ID = 767; + + /** + * A leftover resource that turns out to be already gone must not fail the create: the repository can report it + * as present from a cached view of a storage partition whose resources were deleted by another node. + */ + @Test + public void createSucceedsWhenLeftoverResourceIsAlreadyGone() throws Exception { + Fixture fixture = new Fixture(); + doThrow(HyracksDataException.create(ErrorCode.RESOURCE_DOES_NOT_EXIST, RESOURCE_PATH)) + .when(fixture.localResourceRepository).delete(RESOURCE_PATH); + fixture.builder.build(); + verify(fixture.index).create(); + verify(fixture.localResourceRepository).insert(any()); + verify(fixture.lcManager).register(RESOURCE_PATH, fixture.index); + } + + /** + * Any other failure to clear the leftover resource must still fail the create. + */ + @Test + public void createFailsWhenLeftoverResourceCannotBeDeleted() throws Exception { + Fixture fixture = new Fixture(); + doThrow(HyracksDataException.create(ErrorCode.CANNOT_DELETE_FILE, RESOURCE_PATH)) + .when(fixture.localResourceRepository).delete(RESOURCE_PATH); + HyracksDataException failure = Assert.assertThrows(HyracksDataException.class, () -> fixture.builder.build()); + Assert.assertTrue(failure.matches(ErrorCode.CANNOT_DELETE_FILE)); + verify(fixture.index, never()).create(); + verify(fixture.localResourceRepository, never()).insert(any()); + } + + private static class Fixture { + + private final ILocalResourceRepository localResourceRepository = mock(ILocalResourceRepository.class); + private final IResourceLifecycleManager<IIndex> lcManager = mock(IResourceLifecycleManager.class); + private final IIndex index = mock(IIndex.class); + private final IndexBuilder builder; + + @SuppressWarnings("unchecked") + Fixture() throws HyracksDataException { + INCServiceContext ctx = mock(INCServiceContext.class); + IIOManager ioManager = mock(IIOManager.class); + FileReference resourceRef = mock(FileReference.class); + FileReference resolvedResourceRef = mock(FileReference.class); + IStorageManager storageManager = mock(IStorageManager.class); + IResourceIdFactory resourceIdFactory = mock(IResourceIdFactory.class); + IResourceFactory localResourceFactory = mock(IResourceFactory.class); + IResource resource = mock(IResource.class); + LocalResource leftover = mock(LocalResource.class); + + when(ctx.getIoManager()).thenReturn(ioManager); + when(resourceRef.getRelativePath()).thenReturn(RESOURCE_PATH); + when(ioManager.resolve(RESOURCE_PATH)).thenReturn(resolvedResourceRef); + // the index files are gone along with the resource + when(resolvedResourceRef.getFile()).thenReturn(new File(RESOURCE_PATH)); + when(storageManager.getLifecycleManager(ctx)).thenReturn(lcManager); + when(storageManager.getLocalResourceRepository(ctx)).thenReturn(localResourceRepository); + // the repository reports a leftover resource at the path the index is about to be created at + when(leftover.getId()).thenReturn(LEFTOVER_RESOURCE_ID); + when(localResourceRepository.get(RESOURCE_PATH)).thenReturn(leftover); + when(resourceIdFactory.createId()).thenReturn(NEW_RESOURCE_ID); + when(localResourceFactory.createResource(resourceRef)).thenReturn(resource); + when(resource.createInstance(ctx)).thenReturn(index); + when(lcManager.get(anyString())).thenReturn(null); + + builder = new IndexBuilder(ctx, storageManager, resourceIdFactory, resourceRef, localResourceFactory, true); + } + } +} -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21656?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ieede4752d680832e7801929d26744720a3c05244 Gerrit-Change-Number: 21656 Gerrit-PatchSet: 1 Gerrit-Owner: Murtadha Hubail <[email protected]>
