Skip to content

HDDS-13170. Reclaimable filter should always reclaim entries when buc… #568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jojochuang wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HDDS-13170. Reclaimable filter should always reclaim entries when buc… #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions ...e-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.hadoop.ozone.om.OmSnapshotManager;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.SnapshotChainManager;
import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock;
Expand Down Expand Up @@ -167,11 +168,21 @@ private void initializePreviousSnapshotsFromChain(String volume, String bucket)
previousOmSnapshots.add(null);
previousSnapshotInfos.add(null);
}

// NOTE: Getting volumeId and bucket from active OM.
// This would be wrong on volume & bucket renames support.
bucketInfo = ozoneManager.getBucketInfo(volume, bucket);
}
// NOTE: Getting volumeId and bucket from active OM.
// This would be wrong on volume & bucket renames support.
try {
bucketInfo = ozoneManager.getBucketManager().getBucketInfo(volume, bucket);
volumeId = ozoneManager.getMetadataManager().getVolumeId(volume);
} catch (OMException e) {
// If Volume or bucket has been deleted then all keys should be reclaimable as no snapshots would exist.
if (OMException.ResultCodes.VOLUME_NOT_FOUND == e.getResult() ||
OMException.ResultCodes.BUCKET_NOT_FOUND == e.getResult()) {
bucketInfo = null;
volumeId = null;
return;
}
throw e;
}
} catch (IOException e) {
this.cleanup();
Expand All @@ -187,7 +198,7 @@ public synchronized Boolean apply(Table.KeyValue<String, V> keyValue) throws IOE
if (!validateExistingLastNSnapshotsInChain(volume, bucket) || !snapshotIdLocks.isLockAcquired()) {
initializePreviousSnapshotsFromChain(volume, bucket);
}
boolean isReclaimable = isReclaimable(keyValue);
boolean isReclaimable = (bucketInfo == null) || isReclaimable(keyValue);
// This is to ensure the reclamation ran on the same previous snapshot and no change occurred in the chain
// while processing the entry.
return isReclaimable && validateExistingLastNSnapshotsInChain(volume, bucket);
Expand Down
27 changes: 19 additions & 8 deletions ...c/test/java/org/apache/hadoop/ozone/om/snapshot/filter/AbstractReclaimableFilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,14 @@
import org.apache.hadoop.hdds.utils.db.RDBStore;
import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.hdds.utils.db.managed.ManagedRocksDB;
import org.apache.hadoop.ozone.om.BucketManager;
import org.apache.hadoop.ozone.om.KeyManager;
import org.apache.hadoop.ozone.om.OMMetadataManager;
import org.apache.hadoop.ozone.om.OmSnapshot;
import org.apache.hadoop.ozone.om.OmSnapshotManager;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.SnapshotChainManager;
import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.BucketLayout;
import org.apache.hadoop.ozone.om.helpers.OmBucketInfo;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
Expand Down Expand Up @@ -159,19 +161,28 @@ protected void teardown() throws IOException {

private void mockOzoneManager(BucketLayout bucketLayout) throws IOException {
OMMetadataManager metadataManager = mock(OMMetadataManager.class);
BucketManager bucketManager = mock(BucketManager.class);
when(ozoneManager.getMetadataManager()).thenReturn(metadataManager);
when(ozoneManager.getBucketManager()).thenReturn(bucketManager);
long volumeCount = 0;
long bucketCount = 0;
for (String volume : volumes) {
when(metadataManager.getVolumeId(eq(volume))).thenReturn(volumeCount);
for (String bucket : buckets) {
when(ozoneManager.getBucketInfo(eq(volume), eq(bucket)))
.thenReturn(OmBucketInfo.newBuilder().setVolumeName(volume).setBucketName(bucket)
.setObjectID(bucketCount).setBucketLayout(bucketLayout).build());
bucketCount++;
}
volumeCount++;
}

when(bucketManager.getBucketInfo(anyString(), anyString())).thenAnswer(i -> {
String volume = i.getArgument(0, String.class);
String bucket = i.getArgument(1, String.class);
if (!volumes.contains(volume)) {
throw new OMException("Volume " + volume + " already exists", OMException.ResultCodes.VOLUME_NOT_FOUND);
}
if (!buckets.contains(bucket)) {
throw new OMException("Bucket " + bucket + " already exists", OMException.ResultCodes.BUCKET_NOT_FOUND);
}
Comment on lines +176 to +181
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The exception messages for VOLUME_NOT_FOUND and BUCKET_NOT_FOUND in the mock setup seem to be misleading. For example, for VOLUME_NOT_FOUND, the message is "Volume " + volume + " already exists". Shouldn't this be more like "Volume " + volume + " not found" to accurately reflect the exception code being thrown?

This could cause confusion during debugging if a test fails due to these specific exceptions.

return OmBucketInfo.newBuilder().setVolumeName(volume).setBucketName(bucket)
.setObjectID((long) volumes.indexOf(volume) * buckets.size() + buckets.indexOf(bucket))
.setBucketLayout(bucketLayout).build();
});
}

private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOException {
Expand Down Expand Up @@ -232,7 +243,7 @@ private void mockOmSnapshotManager(OzoneManager om) throws RocksDBException, IOE

protected List<SnapshotInfo> getLastSnapshotInfos(
String volume, String bucket, int numberOfSnapshotsInChain, int index) {
List<SnapshotInfo> infos = getSnapshotInfos().get(getKey(volume, bucket));
List<SnapshotInfo> infos = getSnapshotInfos().getOrDefault(getKey(volume, bucket), Collections.emptyList());
int endIndex = Math.min(index - 1, infos.size() - 1);
return IntStream.range(endIndex - numberOfSnapshotsInChain + 1, endIndex + 1).mapToObj(i -> i >= 0 ?
infos.get(i) : null).collect(Collectors.toList());
Expand Down
2 changes: 1 addition & 1 deletion ...er/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableDirFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private void testReclaimableDirFilter(String volume, String bucket, int index,
List<SnapshotInfo> snapshotInfos = getLastSnapshotInfos(volume, bucket, 1, index);
assertEquals(snapshotInfos.size(), 1);
SnapshotInfo prevSnapshotInfo = snapshotInfos.get(0);
OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket);
OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket);
long volumeId = getOzoneManager().getMetadataManager().getVolumeId(volume);
KeyManager keyManager = getKeyManager();
if (prevSnapshotInfo != null) {
Expand Down
30 changes: 30 additions & 0 deletions ...nager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,36 @@ public void testReclaimableFilterSnapshotChainInitialization(
false);
}

@ParameterizedTest
@MethodSource("testReclaimableFilterArguments")
public void testReclaimableFilterSnapshotChainInitializationWithInvalidVolume(
int numberOfPreviousSnapshotsFromChain, int actualNumberOfSnapshots)
throws IOException, RocksDBException {
SnapshotInfo currentSnapshotInfo =
setup(numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots, actualNumberOfSnapshots + 1, 4, 2);
String volume = "volume" + 6;
String bucket = getBuckets().get(1);
testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1,
currentSnapshotInfo, true, true);
testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1,
currentSnapshotInfo, false, true);
}

@ParameterizedTest
@MethodSource("testReclaimableFilterArguments")
public void testReclaimableFilterSnapshotChainInitializationWithInvalidBucket(
int numberOfPreviousSnapshotsFromChain, int actualNumberOfSnapshots)
throws IOException, RocksDBException {
SnapshotInfo currentSnapshotInfo =
setup(numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots, actualNumberOfSnapshots + 1, 4, 2);
String volume = getVolumes().get(3);
String bucket = "bucket" + 6;
testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1,
currentSnapshotInfo, true, true);
testSnapshotInitAndLocking(volume, bucket, numberOfPreviousSnapshotsFromChain, actualNumberOfSnapshots + 1,
currentSnapshotInfo, false, true);
}

@ParameterizedTest
@MethodSource("testReclaimableFilterArguments")
public void testReclaimableFilterWithBucketVolumeMismatch(
Expand Down
2 changes: 1 addition & 1 deletion ...er/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ private void testReclaimableKeyFilter(String volume, String bucket, int index,
List<SnapshotInfo> snapshotInfos = getLastSnapshotInfos(volume, bucket, 2, index);
SnapshotInfo previousToPreviousSapshotInfo = snapshotInfos.get(0);
SnapshotInfo prevSnapshotInfo = snapshotInfos.get(1);
OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket);
OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket);
long volumeId = getOzoneManager().getMetadataManager().getVolumeId(volume);

UncheckedAutoCloseableSupplier<OmSnapshot> prevSnap = Optional.ofNullable(prevSnapshotInfo)
Expand Down
2 changes: 1 addition & 1 deletion ...est/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableRenameEntryFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private void testReclaimableRenameEntryFilter(String volume, String bucket, int
throws IOException {
List<SnapshotInfo> snapshotInfos = getLastSnapshotInfos(volume, bucket, 1, index);
SnapshotInfo prevSnapshotInfo = snapshotInfos.get(0);
OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket);
OmBucketInfo bucketInfo = getOzoneManager().getBucketManager().getBucketInfo(volume, bucket);
if (prevSnapshotInfo != null) {
UncheckedAutoCloseableSupplier<OmSnapshot> prevSnap = Optional.ofNullable(prevSnapshotInfo)
.map(info -> {
Expand Down
Loading