-
Notifications
You must be signed in to change notification settings - Fork 589
HDDS-12561. Reclaimable Rename entry filter for reclaiming renaming entries #8054
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.
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
Changes from all commits
7327b47
bd5b0c6
d4a2f07
f1c85fd
01c52b9
43ab7b7
51c88f1
d27bd24
b901166
690eae9
865f3a5
5743edb
abcfaff
a2127a4
9f6d2a0
9d6bae3
7d785ab
e1d2317
5eca81d
c4312b1
93ba939
3935bf8
6d638a0
681bb3d
8fd746c
b69182e
d32bcf0
f8a2b6e
f323e3e
29a26d2
1ebba73
0010107
9317970
52b8864
6436166
591eceb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,93 @@ | ||||||
| /* | ||||||
| * 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.hadoop.ozone.om.snapshot.filter; | ||||||
|
|
||||||
| import java.io.IOException; | ||||||
| import org.apache.hadoop.hdds.utils.db.Table; | ||||||
| import org.apache.hadoop.ozone.om.KeyManager; | ||||||
| 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.helpers.OmDirectoryInfo; | ||||||
| import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; | ||||||
| import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; | ||||||
| import org.apache.hadoop.ozone.om.helpers.WithObjectID; | ||||||
| import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock; | ||||||
| import org.apache.hadoop.ozone.om.snapshot.ReferenceCounted; | ||||||
|
|
||||||
| /** | ||||||
| * Filter to return rename table entries which are reclaimable based on the key presence in previous snapshot's | ||||||
| * keyTable/DirectoryTable in the snapshot chain. | ||||||
| */ | ||||||
| public class ReclaimableRenameEntryFilter extends ReclaimableFilter<String> { | ||||||
|
|
||||||
| public ReclaimableRenameEntryFilter(OzoneManager ozoneManager, | ||||||
| OmSnapshotManager omSnapshotManager, SnapshotChainManager snapshotChainManager, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra indentation. our checkstyle rule allow this? |
||||||
| SnapshotInfo currentSnapshotInfo, KeyManager keyManager, | ||||||
| IOzoneManagerLock lock) { | ||||||
| super(ozoneManager, omSnapshotManager, snapshotChainManager, currentSnapshotInfo, keyManager, lock, 1); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Function which checks whether the objectId corresponding to the rename entry exists in the previous snapshot. If | ||||||
| * the entry doesn't exist in the previous keyTable/directoryTable then the rename entry can be deleted since there | ||||||
| * is no reference for this rename entry. | ||||||
| * @return true if there is no reference for the objectId based on the rename entry otherwise false. | ||||||
| * @throws IOException | ||||||
| */ | ||||||
| @Override | ||||||
| protected Boolean isReclaimable(Table.KeyValue<String, String> renameEntry) throws IOException { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a comment explaining how it works would be idea.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
| ReferenceCounted<OmSnapshot> previousSnapshot = getPreviousOmSnapshot(0); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the index always supposed to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah. There is going to be only 1 snapshot in the previous snapshot chain. |
||||||
| Table<String, OmKeyInfo> previousKeyTable = null; | ||||||
| Table<String, OmDirectoryInfo> prevDirTable = null; | ||||||
| if (previousSnapshot != null) { | ||||||
| previousKeyTable = previousSnapshot.get().getMetadataManager().getKeyTable(getBucketInfo().getBucketLayout()); | ||||||
| if (getBucketInfo().getBucketLayout().isFileSystemOptimized()) { | ||||||
| prevDirTable = previousSnapshot.get().getMetadataManager().getDirectoryTable(); | ||||||
| } | ||||||
| } | ||||||
| return isRenameEntryReclaimable(renameEntry, prevDirTable, previousKeyTable); | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected String getVolumeName(Table.KeyValue<String, String> keyValue) throws IOException { | ||||||
| return getKeyManager().getMetadataManager().splitRenameKey(keyValue.getKey())[0]; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO it should be just
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected String getBucketName(Table.KeyValue<String, String> keyValue) throws IOException { | ||||||
| return getKeyManager().getMetadataManager().splitRenameKey(keyValue.getKey())[1]; | ||||||
| } | ||||||
|
|
||||||
| @SafeVarargs | ||||||
| private final boolean isRenameEntryReclaimable(Table.KeyValue<String, String> renameEntry, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to have an understanding on this. Are we just checking if the renamed entry is in the previous snapshot's keyTable and dirTable? Should the check be separate of keys and directories?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are checking if a rename entry points to an object in the previous snapshot. Since objectId is unique in the system the entry would be either in the directoryTable or fileTable or keyTable |
||||||
| Table<String, ? extends WithObjectID>... previousTables) | ||||||
| throws IOException { | ||||||
| for (Table<String, ? extends WithObjectID> previousTable : previousTables) { | ||||||
| if (previousTable != null) { | ||||||
| String prevDbKey = renameEntry.getValue(); | ||||||
| WithObjectID withObjectID = previousTable.getIfExist(prevDbKey); | ||||||
| if (withObjectID != null) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the rename entry is found in any of the previous tables, return false because it cannot be reclaimed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we cannot delete the rename entry |
||||||
| return false; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| /* | ||
| * 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.hadoop.ozone.om.snapshot.filter; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.mockito.ArgumentMatchers.anyString; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import com.google.common.collect.ImmutableMap; | ||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.UUID; | ||
| import org.apache.hadoop.hdds.utils.db.Table; | ||
| 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.helpers.BucketLayout; | ||
| import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; | ||
| import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo; | ||
| import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; | ||
| import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; | ||
| import org.apache.hadoop.ozone.om.lock.IOzoneManagerLock; | ||
| import org.apache.hadoop.ozone.om.snapshot.ReferenceCounted; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
| import org.rocksdb.RocksDBException; | ||
|
|
||
| /** | ||
| * Test class for ReclaimableRenameEntryFilter. | ||
| */ | ||
| public class TestReclaimableRenameEntryFilter extends AbstractReclaimableFilterTest { | ||
| @Override | ||
| protected ReclaimableFilter initializeFilter(OzoneManager om, OmSnapshotManager snapshotManager, | ||
| SnapshotChainManager chainManager, SnapshotInfo currentSnapshotInfo, | ||
| KeyManager km, IOzoneManagerLock lock, | ||
| int numberOfPreviousSnapshotsFromChain) { | ||
| return new ReclaimableRenameEntryFilter(om, snapshotManager, chainManager, currentSnapshotInfo, km, lock); | ||
| } | ||
|
|
||
| List<Arguments> testReclaimableFilterArguments() { | ||
| List<Arguments> arguments = new ArrayList<>(); | ||
| for (int i = 0; i < 3; i++) { | ||
| for (int j = 0; j < 5; j++) { | ||
| arguments.add(Arguments.of(i, j)); | ||
| } | ||
| } | ||
| return arguments; | ||
| } | ||
|
|
||
| private void testReclaimableRenameEntryFilter(String volume, String bucket, int index, | ||
| String value, | ||
| Table<String, OmKeyInfo> keyTable, | ||
| Table<String, OmDirectoryInfo> dirTable, | ||
| Boolean expectedValue) | ||
| throws IOException { | ||
| List<SnapshotInfo> snapshotInfos = getLastSnapshotInfos(volume, bucket, 1, index); | ||
| SnapshotInfo prevSnapshotInfo = snapshotInfos.get(0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. look at just one nearest previous snapshot.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We just have to look if a key corresponding to the object is present in the previous snapshot or not. That is what the rename table tracks. It just keeps a note of the reference of the object in the previous snapshot's keyTable/fileTable/directoryTable. If the entry doesn't exist then we can very well delete this entry |
||
| OmBucketInfo bucketInfo = getOzoneManager().getBucketInfo(volume, bucket); | ||
| if (prevSnapshotInfo != null) { | ||
| ReferenceCounted<OmSnapshot> prevSnap = Optional.ofNullable(prevSnapshotInfo) | ||
| .map(info -> { | ||
| try { | ||
| return getOmSnapshotManager().getActiveSnapshot(volume, bucket, info.getName()); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }).orElse(null); | ||
| mockOmSnapshot(prevSnap, bucketInfo, keyTable, dirTable); | ||
| } | ||
| String key = bucketInfo.getVolumeName() + "/" + bucketInfo.getBucketName() + "/" + 1; | ||
| String[] keySplit = key.split("/"); | ||
| KeyManager km = getKeyManager(); | ||
| OMMetadataManager omMetadataManager = mock(OMMetadataManager.class); | ||
| when(km.getMetadataManager()).thenReturn(omMetadataManager); | ||
| when(omMetadataManager.splitRenameKey(eq(key))).thenReturn(keySplit); | ||
| assertEquals(expectedValue, getReclaimableFilter().apply(Table.newKeyValue(key, value))); | ||
| } | ||
|
|
||
| private <T> Table<String, T> getMockedTable(Map<String, T> map) throws IOException { | ||
| Table<String, T> table = mock(Table.class); | ||
| when(table.get(anyString())).thenAnswer(i -> map.get(i.getArgument(0))); | ||
| when(table.getIfExist(anyString())).thenAnswer(i -> map.get(i.getArgument(0))); | ||
| return table; | ||
| } | ||
|
|
||
| private <T> Table<String, T> getFailingMockedTable() throws IOException { | ||
| Table<String, T> table = mock(Table.class); | ||
| when(table.get(anyString())).thenThrow(new IOException()); | ||
| when(table.getIfExist(anyString())).thenThrow(new IOException()); | ||
| return table; | ||
| } | ||
|
|
||
| private void mockOmSnapshot(ReferenceCounted<OmSnapshot> snapshot, | ||
| OmBucketInfo bucketInfo, Table<String, OmKeyInfo> keyTable, | ||
| Table<String, OmDirectoryInfo> dirTable) { | ||
| if (snapshot != null) { | ||
| OmSnapshot omSnapshot = snapshot.get(); | ||
| OMMetadataManager omMetadataManager = mock(OMMetadataManager.class); | ||
| when(omSnapshot.getMetadataManager()).thenReturn(omMetadataManager); | ||
| when(omMetadataManager.getKeyTable(eq(bucketInfo.getBucketLayout()))).thenReturn(keyTable); | ||
| when(omMetadataManager.getDirectoryTable()).thenReturn(dirTable); | ||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testReclaimableFilterArguments") | ||
| public void testNonReclaimableRenameEntryWithKeyNonFSO(int actualNumberOfSnapshots, int index) | ||
| throws IOException, RocksDBException { | ||
| setup(1, actualNumberOfSnapshots, index, 4, 2, | ||
| BucketLayout.OBJECT_STORE); | ||
| String volume = getVolumes().get(3); | ||
| String bucket = getBuckets().get(1); | ||
| index = Math.min(index, actualNumberOfSnapshots); | ||
| String value = UUID.randomUUID().toString(); | ||
| Table<String, OmKeyInfo> keyTable = getMockedTable(ImmutableMap.of(value, mock(OmKeyInfo.class))); | ||
| Table<String, OmDirectoryInfo> directoryTable = getFailingMockedTable(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why make directory table throw exception?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to ensure keyTable if the entry exists in the keyTable then it shouldn't even call directory table existence just checking if it existed in keyTable then it shouldn't go to directory table |
||
| testReclaimableRenameEntryFilter(volume, bucket, index, value, keyTable, directoryTable, index == 0); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to understand these test suites. What does index here mean and why should it be this condition?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only be able to reclaim the entry if the snapshot is the first snapshot in the chain.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also reclaimable if no snapshot at all? index = Math.min(index, actualNumberOfSnapshots); |
||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testReclaimableFilterArguments") | ||
| public void testReclaimableRenameEntryWithKeyNonFSO(int actualNumberOfSnapshots, int index) | ||
| throws IOException, RocksDBException { | ||
| setup(1, actualNumberOfSnapshots, index, 4, 2, | ||
| BucketLayout.OBJECT_STORE); | ||
| String volume = getVolumes().get(3); | ||
| String bucket = getBuckets().get(1); | ||
| index = Math.min(index, actualNumberOfSnapshots); | ||
| String value = UUID.randomUUID().toString(); | ||
| Table<String, OmKeyInfo> keyTable = getMockedTable(Collections.emptyMap()); | ||
| Table<String, OmDirectoryInfo> directoryTable = getFailingMockedTable(); | ||
| testReclaimableRenameEntryFilter(volume, bucket, index, value, keyTable, directoryTable, true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so always reclaimable if the key is not in the snapshot (key table is empty)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testReclaimableFilterArguments") | ||
| public void testReclaimableRenameEntryWithFSO(int actualNumberOfSnapshots, int index) | ||
| throws IOException, RocksDBException { | ||
| setup(1, actualNumberOfSnapshots, index, 4, 2, | ||
| BucketLayout.FILE_SYSTEM_OPTIMIZED); | ||
| String volume = getVolumes().get(3); | ||
| String bucket = getBuckets().get(1); | ||
| index = Math.min(index, actualNumberOfSnapshots); | ||
| String value = UUID.randomUUID().toString(); | ||
| Table<String, OmKeyInfo> keyTable = getMockedTable(Collections.emptyMap()); | ||
| Table<String, OmDirectoryInfo> directoryTable = getMockedTable(Collections.emptyMap()); | ||
| testReclaimableRenameEntryFilter(volume, bucket, index, value, keyTable, directoryTable, true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. always reclaimable if both key table and directory table are empty in a FSO bucket.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah |
||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testReclaimableFilterArguments") | ||
| public void testNonReclaimableRenameEntryWithFileFSO(int actualNumberOfSnapshots, int index) | ||
| throws IOException, RocksDBException { | ||
| setup(1, actualNumberOfSnapshots, index, 4, 2, | ||
| BucketLayout.FILE_SYSTEM_OPTIMIZED); | ||
| String volume = getVolumes().get(3); | ||
| String bucket = getBuckets().get(1); | ||
| index = Math.min(index, actualNumberOfSnapshots); | ||
| String value = UUID.randomUUID().toString(); | ||
| Table<String, OmKeyInfo> keyTable = getMockedTable(ImmutableMap.of(value, mock(OmKeyInfo.class))); | ||
| Table<String, OmDirectoryInfo> directoryTable = getMockedTable(Collections.emptyMap()); | ||
| testReclaimableRenameEntryFilter(volume, bucket, index, value, keyTable, directoryTable, index == 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reclaimable if key is in the key table and its the first snapshot (or there's no snapshot) in a FSO bucket.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah since there is no previous snapshot |
||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testReclaimableFilterArguments") | ||
| public void testNonReclaimableRenameEntryWithDirFSO(int actualNumberOfSnapshots, int index) | ||
| throws IOException, RocksDBException { | ||
| setup(1, actualNumberOfSnapshots, index, 4, 2, | ||
| BucketLayout.FILE_SYSTEM_OPTIMIZED); | ||
| String volume = getVolumes().get(3); | ||
| String bucket = getBuckets().get(1); | ||
| index = Math.min(index, actualNumberOfSnapshots); | ||
| String value = UUID.randomUUID().toString(); | ||
| Table<String, OmKeyInfo> keyTable = getMockedTable(Collections.emptyMap()); | ||
| Table<String, OmDirectoryInfo> directoryTable = getMockedTable(ImmutableMap.of(value, mock(OmDirectoryInfo.class))); | ||
| testReclaimableRenameEntryFilter(volume, bucket, index, value, keyTable, directoryTable, index == 0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reclaimable if key is in the key table or directory table and its the first snapshot (or there's no snapshot) in a FSO bucket. |
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're getting into the habit of creating helper methods inside OMMetadataManager that are unrelated to OMMetadataManager internals. IMO it (and others) should go into a utility class, like SnapshotUtil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not do it as part of a util function as this is completely dependent on how the metadata has been layed out. Rather having it in the snapshotUtil is a bad idea as we are mixing up metadata layout internal stuff outside of this metdata layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jojochuang I don't like the whole idea of Snapshots using MetadataManager directly, right now all of our code is tightly coupled with metadatamanager. Like other flows snapshots are supposed to use KeyManager & BucketManager etc. These set of patches would cleanup these shortcomings in our implementation. I @prashantpogde & @hemantk-12 had discussed this and aligned to eventually get there. If we start using MetadataManager we are completely defeating the purpose of higher level abstractions.