You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/03/23 10:20:46 UTC
[GitHub] [ozone] JyotinderSingh opened a new pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
JyotinderSingh opened a new pull request #3227:
URL: https://github.com/apache/ozone/pull/3227
## What changes were proposed in this pull request?
OmMetadataManagerImpl#isBucketEmpty only checks the key table. This means it is possible to delete an FSO bucket that still has data, orphaning the remaining data on the OM.
This patch adds a new code path for File System Optimized buckets. In the case of FSO layout, we check the directory and file tables for any entries that might be present under the said bucket.
In buckets with FSO layout, the metadata entry for a
- **Top-level directory** would be of the format `<bucket ID>/dirName` inside the `dirTable`.
- **Top-level file** (a file directly placed under the bucket without any sub paths) would be of the format `<bucket ID>/fileName` inside the `fileTable`.
We seek any keys in these tables that begin with the requested bucket's ID - if we find any such entries in the table or the table cache, we return False.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6425
## How was this patch tested?
Added integration test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r834185500
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -707,46 +707,104 @@ public boolean isVolumeEmpty(String volume) throws IOException {
@Override
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
- String keyPrefix = getBucketKey(volume, bucket);
+ String bucketKey = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(bucketKey);
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
+
+ // keyPrefix is different in case of fileTable and keyTable.
+ // 1. For OBS and LEGACY buckets:
+ // the entries are present in the keyTable and are of the
+ // format <bucketKey>/<key-name>
+ // 2. For FSO buckets:
+ // - TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // - TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+ String keyPrefix =
+ bucketLayout.isFileSystemOptimized() ? bucketId : bucketKey;
+
+ // Check key/file Table
+ Table<String, OmKeyInfo> table = getKeyTable(bucketLayout);
+
+ // First check in table cache.
+ if (isKeyPresentInTableCache(keyPrefix, table)) {
+ return false;
+ }
+ // check in table
+ if (isKeyPresentInTable(keyPrefix, table)) {
+ return false; // we found at least one key with this vol/bucket
+ }
- // First check in key table cache.
+ // Check dirTable as well in case of FSO bucket.
+ if (bucketLayout.isFileSystemOptimized()) {
+ // First check in dirTable cache
+ if (isKeyPresentInTableCache(keyPrefix, dirTable)) {
+ return false;
+ }
+ // Check in the table
+ return !isKeyPresentInTable(keyPrefix, dirTable);
+ }
+ return true;
+ }
+
+
+ /**
+ * Checks if a key starting with a given keyPrefix exists in the table cache.
+ *
+ * @param keyPrefix - key prefix to be searched.
+ * @param table - table to be searched.
+ * @return true if the key is present in the cache.
+ */
+ private boolean isKeyPresentInTableCache(String keyPrefix,
+ Table table) {
Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
+ table.cacheIterator();
while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
iterator.next();
String key = entry.getKey().getCacheKey();
OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
// Making sure that entry is not for delete key request.
if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
+ return true;
}
}
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
+ return false;
+ }
+
+ /**
+ * Checks if a key starts with the given prefix is present in the table.
+ *
+ * @param keyPrefix - Prefix to check for
+ * @param table - Table to check in
+ * @return true if the key is present in the table
+ * @throws IOException
+ */
+ private boolean isKeyPresentInTable(String keyPrefix,
+ Table<String, OmKeyInfo> table)
+ throws IOException {
+ try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
if (kv != null) {
// Check the entry in db is not marked for delete. This can happen
// while entry is marked for delete, but it is not flushed to DB.
CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
+ table.getCacheValue(new CacheKey(kv.getKey()));
if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
- }
+ // we found at least one key with this prefix.
+ return kv.getKey().startsWith(keyPrefix)
+ && cacheValue.getCacheValue() != null;
Review comment:
I am not sure this method is correct. Is your intention, that if you find a key in the table iterator, but there is a "delete marker" in the cache, then treat the entry as if it is deleted?
If you find an entry and not cache entry, we return true.
If we find an entry and there is a cache entry:
If the cache entry is not null we return true.
If the cache entry is null, we return false.
However, what if there is another key with the same prefix that is not deleted in the cache? Do we not have to keep iterating until we get past the prefix range we care about?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr merged pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
rakeshadr merged pull request #3227:
URL: https://github.com/apache/ozone/pull/3227
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833900958
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1455,6 +1458,9 @@ public void testRenameFile() throws Exception {
assertTrue("Renamed failed: /dir/file1", getFs().exists(file1Destin));
FileStatus[] fStatus = getFs().listStatus(dirPath);
assertEquals("Renamed failed", 1, fStatus.length);
+
+ // We cannot delete a non-empty bucket.
+ getFs().delete(dirPath, true);
Review comment:
Yes you are right, I just realised that the test was passing earlier because of incorrect isBucketEmpty() implementation for FSO, please revert the change, We can correct these in HDDS-6414
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JyotinderSingh commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r834369769
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -707,46 +707,104 @@ public boolean isVolumeEmpty(String volume) throws IOException {
@Override
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
- String keyPrefix = getBucketKey(volume, bucket);
+ String bucketKey = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(bucketKey);
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
+
+ // keyPrefix is different in case of fileTable and keyTable.
+ // 1. For OBS and LEGACY buckets:
+ // the entries are present in the keyTable and are of the
+ // format <bucketKey>/<key-name>
+ // 2. For FSO buckets:
+ // - TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // - TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+ String keyPrefix =
+ bucketLayout.isFileSystemOptimized() ? bucketId : bucketKey;
+
+ // Check key/file Table
+ Table<String, OmKeyInfo> table = getKeyTable(bucketLayout);
+
+ // First check in table cache.
+ if (isKeyPresentInTableCache(keyPrefix, table)) {
+ return false;
+ }
+ // check in table
+ if (isKeyPresentInTable(keyPrefix, table)) {
+ return false; // we found at least one key with this vol/bucket
+ }
- // First check in key table cache.
+ // Check dirTable as well in case of FSO bucket.
+ if (bucketLayout.isFileSystemOptimized()) {
+ // First check in dirTable cache
+ if (isKeyPresentInTableCache(keyPrefix, dirTable)) {
+ return false;
+ }
+ // Check in the table
+ return !isKeyPresentInTable(keyPrefix, dirTable);
+ }
+ return true;
+ }
+
+
+ /**
+ * Checks if a key starting with a given keyPrefix exists in the table cache.
+ *
+ * @param keyPrefix - key prefix to be searched.
+ * @param table - table to be searched.
+ * @return true if the key is present in the cache.
+ */
+ private boolean isKeyPresentInTableCache(String keyPrefix,
+ Table table) {
Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
+ table.cacheIterator();
while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
iterator.next();
String key = entry.getKey().getCacheKey();
OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
// Making sure that entry is not for delete key request.
if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
+ return true;
}
}
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
+ return false;
+ }
+
+ /**
+ * Checks if a key starts with the given prefix is present in the table.
+ *
+ * @param keyPrefix - Prefix to check for
+ * @param table - Table to check in
+ * @return true if the key is present in the table
+ * @throws IOException
+ */
+ private boolean isKeyPresentInTable(String keyPrefix,
+ Table<String, OmKeyInfo> table)
+ throws IOException {
+ try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
if (kv != null) {
// Check the entry in db is not marked for delete. This can happen
// while entry is marked for delete, but it is not flushed to DB.
CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
+ table.getCacheValue(new CacheKey(kv.getKey()));
if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
- }
+ // we found at least one key with this prefix.
+ return kv.getKey().startsWith(keyPrefix)
+ && cacheValue.getCacheValue() != null;
Review comment:
That's a good point, this case skipped my mind. I have updated the code to loop over the said prefix range to make sure we look at all such keys.
Please let me know if it looks good to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833561516
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestObjectStoreWithFSO.java
##########
@@ -213,6 +213,79 @@ public void testCreateKey() throws Exception {
dirPathC.getObjectID(), true);
}
+ /**
+ * Tests bucket deletion behaviour. Buckets should not be allowed to be
+ * deleted if they contain files or directories under them.
+ *
+ * @throws Exception
+ */
+ @Test
+ public void testBucketWithKeyDelete() throws Exception {
+ // Create temporary volume and bucket for this test.
+ OzoneBucket testBucket = TestDataUtil
+ .createVolumeAndBucket(cluster, BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ String testVolumeName = testBucket.getVolumeName();
+ String testBucketName = testBucket.getName();
+
+ String parent = "a/b/c/";
+ String file = "key" + RandomStringUtils.randomNumeric(5);
+ String key = parent + file;
+
+ OzoneClient client = cluster.getClient();
+
+ ObjectStore objectStore = client.getObjectStore();
+ OzoneVolume ozoneVolume = objectStore.getVolume(testVolumeName);
+ assertEquals(ozoneVolume.getName(), testVolumeName);
+ OzoneBucket ozoneBucket = ozoneVolume.getBucket(testBucketName);
+ assertEquals(ozoneBucket.getName(), testBucketName);
+
+ Table<String, OmKeyInfo> openFileTable =
+ cluster.getOzoneManager().getMetadataManager()
+ .getOpenKeyTable(getBucketLayout());
+
+ // before file creation
+ verifyKeyInFileTable(openFileTable, file, 0, true);
+
+ // Create a key.
+ ozoneBucket.createKey(key, 10).close();
+
+ try {
+ // Try to delete the bucket while a key is present under it.
+ ozoneVolume.deleteBucket(testBucketName);
+ fail("Bucket Deletion should fail, since bucket is not empty.");
+ } catch (IOException ioe) {
+ // Do nothing
+ }
+
+ // Delete the key (this only deletes the file)
+ ozoneBucket.deleteKey(key);
+ try {
+ // Try to delete the bucket while intermediate dirs are present under it.
+ ozoneVolume.deleteBucket(testBucketName);
+ fail("Bucket Deletion should fail, since bucket still contains " +
+ "intermediate directories");
+ } catch (IOException ioe) {
+ // Do nothing
+ }
+
+ // Delete last level of directories.
+ ozoneBucket.deleteDirectory(parent, true);
+ try {
+ // Try to delete the bucket while dirs are present under it.
+ ozoneVolume.deleteBucket(testBucketName);
+ fail("Bucket Deletion should fail, since bucket still contains " +
+ "intermediate directories");
+ } catch (IOException ioe) {
+ // Do nothing
+ }
+
+ // Delete all the intermediate directories
+ ozoneBucket.deleteDirectory("a/", true);
Review comment:
maybe also add an assertion for isBucketEmpty when bucket is both empty and non empty
```suggestion
ozoneBucket.deleteDirectory("a/", true);
Assert.assertTrue(cluster.getOzoneManager().getMetadataManager()
.isBucketEmpty(testVolumeName, testBucketName));
```
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1455,6 +1458,9 @@ public void testRenameFile() throws Exception {
assertTrue("Renamed failed: /dir/file1", getFs().exists(file1Destin));
FileStatus[] fStatus = getFs().listStatus(dirPath);
assertEquals("Renamed failed", 1, fStatus.length);
+
+ // We cannot delete a non-empty bucket.
+ getFs().delete(dirPath, true);
Review comment:
We can delete a non-empty bucket if recursive is set to true via Filesystem, there is already a check on this [line ](https://github.com/apache/ozone/blob/4ff630082b39ca0ebbb9080913f52f9a7b9d7d18/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java#L1089)for non recursive bucket deletion, I guess we can remove this and other occurrences of this in testRenameFileToDir & testRenameToParentDir, The test added in TestObjectStoreWithFSO should be good. For recursive bucket deletion I can add separate test in HDDS-6414
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JyotinderSingh commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833518029
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -708,43 +708,101 @@ public boolean isVolumeEmpty(String volume) throws IOException {
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
String keyPrefix = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(keyPrefix);
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
- // First check in key table cache.
- Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
- while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
- iterator.next();
- String key = entry.getKey().getCacheKey();
- OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
- // Making sure that entry is not for delete key request.
- if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
- }
- }
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
- KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
-
- if (kv != null) {
- // Check the entry in db is not marked for delete. This can happen
- // while entry is marked for delete, but it is not flushed to DB.
- CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
- if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ // FSO Bucket
+ if (bucketLayout.isFileSystemOptimized()) {
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ // We want to check if there are any directories or files
+ // that are present under this bucket.
+ // In buckets with FSO layout, the metadata entry for a
+ // 1. TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // 2. TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+
+ // Iterate over the keyTable and fileTable
+ for (Table table : new Table[]{dirTable, fileTable}) {
+ // First check in the fileTable and dirTable cache
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ table.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ if (key.startsWith(bucketId) && omKeyInfo != null) {
+ return false;
}
- } else {
- if (kv.getKey().startsWith(keyPrefix)) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ }
+
+ // Check in the table
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
+ KeyValue<String, OmKeyInfo> kv = keyIter.seek(bucketId);
+
+ if (kv != null) {
+ // Check the entry in db is not marked for delete. This can happen
+ // while entry is marked for delete, but it is not flushed to DB.
+ CacheValue<OmKeyInfo> cacheValue =
+ table.getCacheValue(new CacheKey(kv.getKey()));
+ if (cacheValue != null) {
+ if (kv.getKey().startsWith(bucketId)
+ && cacheValue.getCacheValue() != null) {
+ return false; // we found at least one file/dir under this
+ // bucket
+ }
+ } else {
+ if (kv.getKey().startsWith(bucketId)) {
+ return false; // we found at least one file/dir under this
+ // bucket.
+ }
+ }
}
}
}
+ } else {
Review comment:
Thank you for the review @sodonnel, I have addressed your review in my latest patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
sadanand48 commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833453867
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -708,43 +708,101 @@ public boolean isVolumeEmpty(String volume) throws IOException {
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
String keyPrefix = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(keyPrefix);
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
- // First check in key table cache.
- Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
- while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
- iterator.next();
- String key = entry.getKey().getCacheKey();
- OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
- // Making sure that entry is not for delete key request.
- if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
- }
- }
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
- KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
-
- if (kv != null) {
- // Check the entry in db is not marked for delete. This can happen
- // while entry is marked for delete, but it is not flushed to DB.
- CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
- if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ // FSO Bucket
+ if (bucketLayout.isFileSystemOptimized()) {
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ // We want to check if there are any directories or files
+ // that are present under this bucket.
+ // In buckets with FSO layout, the metadata entry for a
+ // 1. TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // 2. TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+
+ // Iterate over the keyTable and fileTable
+ for (Table table : new Table[]{dirTable, fileTable}) {
+ // First check in the fileTable and dirTable cache
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ table.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ if (key.startsWith(bucketId) && omKeyInfo != null) {
+ return false;
}
- } else {
- if (kv.getKey().startsWith(keyPrefix)) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ }
+
+ // Check in the table
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
+ KeyValue<String, OmKeyInfo> kv = keyIter.seek(bucketId);
+
+ if (kv != null) {
+ // Check the entry in db is not marked for delete. This can happen
+ // while entry is marked for delete, but it is not flushed to DB.
+ CacheValue<OmKeyInfo> cacheValue =
+ table.getCacheValue(new CacheKey(kv.getKey()));
+ if (cacheValue != null) {
+ if (kv.getKey().startsWith(bucketId)
+ && cacheValue.getCacheValue() != null) {
+ return false; // we found at least one file/dir under this
+ // bucket
+ }
+ } else {
+ if (kv.getKey().startsWith(bucketId)) {
+ return false; // we found at least one file/dir under this
+ // bucket.
+ }
+ }
}
}
}
+ } else {
+ // OBS or LEGACY bucket
+ // First check in key table cache.
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ ((TypedTable<String, OmKeyInfo>) keyTable).cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ // Making sure that entry is not for delete key request.
+ if (key.startsWith(keyPrefix) && omKeyInfo != null) {
Review comment:
I think it would be better to have a single method like isKeyPresent(Table Table, String keyPrefix) since the search logic is same across all tables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JyotinderSingh commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833518620
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -708,43 +708,101 @@ public boolean isVolumeEmpty(String volume) throws IOException {
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
String keyPrefix = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(keyPrefix);
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
- // First check in key table cache.
- Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
- while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
- iterator.next();
- String key = entry.getKey().getCacheKey();
- OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
- // Making sure that entry is not for delete key request.
- if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
- }
- }
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
- KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
-
- if (kv != null) {
- // Check the entry in db is not marked for delete. This can happen
- // while entry is marked for delete, but it is not flushed to DB.
- CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
- if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ // FSO Bucket
+ if (bucketLayout.isFileSystemOptimized()) {
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ // We want to check if there are any directories or files
+ // that are present under this bucket.
+ // In buckets with FSO layout, the metadata entry for a
+ // 1. TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // 2. TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+
+ // Iterate over the keyTable and fileTable
+ for (Table table : new Table[]{dirTable, fileTable}) {
+ // First check in the fileTable and dirTable cache
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ table.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ if (key.startsWith(bucketId) && omKeyInfo != null) {
+ return false;
}
- } else {
- if (kv.getKey().startsWith(keyPrefix)) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ }
+
+ // Check in the table
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
+ KeyValue<String, OmKeyInfo> kv = keyIter.seek(bucketId);
+
+ if (kv != null) {
+ // Check the entry in db is not marked for delete. This can happen
+ // while entry is marked for delete, but it is not flushed to DB.
+ CacheValue<OmKeyInfo> cacheValue =
+ table.getCacheValue(new CacheKey(kv.getKey()));
+ if (cacheValue != null) {
+ if (kv.getKey().startsWith(bucketId)
+ && cacheValue.getCacheValue() != null) {
+ return false; // we found at least one file/dir under this
+ // bucket
+ }
+ } else {
+ if (kv.getKey().startsWith(bucketId)) {
+ return false; // we found at least one file/dir under this
+ // bucket.
+ }
+ }
}
}
}
+ } else {
+ // OBS or LEGACY bucket
+ // First check in key table cache.
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ ((TypedTable<String, OmKeyInfo>) keyTable).cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ // Making sure that entry is not for delete key request.
+ if (key.startsWith(keyPrefix) && omKeyInfo != null) {
Review comment:
Thank you for the review @sadanand48, I have addressed your comments in my latest patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833391940
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -708,43 +708,101 @@ public boolean isVolumeEmpty(String volume) throws IOException {
public boolean isBucketEmpty(String volume, String bucket)
throws IOException {
String keyPrefix = getBucketKey(volume, bucket);
+ OmBucketInfo omBucketInfo = getBucketTable().get(keyPrefix);
+ BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
- // First check in key table cache.
- Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
- ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
- while (iterator.hasNext()) {
- Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
- iterator.next();
- String key = entry.getKey().getCacheKey();
- OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
- // Making sure that entry is not for delete key request.
- if (key.startsWith(keyPrefix) && omKeyInfo != null) {
- return false;
- }
- }
- try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
- keyTable.iterator()) {
- KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
-
- if (kv != null) {
- // Check the entry in db is not marked for delete. This can happen
- // while entry is marked for delete, but it is not flushed to DB.
- CacheValue<OmKeyInfo> cacheValue =
- keyTable.getCacheValue(new CacheKey(kv.getKey()));
- if (cacheValue != null) {
- if (kv.getKey().startsWith(keyPrefix)
- && cacheValue.getCacheValue() != null) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ // FSO Bucket
+ if (bucketLayout.isFileSystemOptimized()) {
+ String bucketId = String.valueOf(omBucketInfo.getObjectID());
+ // We want to check if there are any directories or files
+ // that are present under this bucket.
+ // In buckets with FSO layout, the metadata entry for a
+ // 1. TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+ // inside the dirTable.
+ // 2. TOP-LEVEL FILE (a file directly placed under the bucket without
+ // any sub paths) would be of the format <bucket ID>/fileName inside
+ // the fileTable.
+
+ // Iterate over the keyTable and fileTable
+ for (Table table : new Table[]{dirTable, fileTable}) {
+ // First check in the fileTable and dirTable cache
+ Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+ table.cacheIterator();
+ while (iterator.hasNext()) {
+ Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+ iterator.next();
+ String key = entry.getKey().getCacheKey();
+ OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+ if (key.startsWith(bucketId) && omKeyInfo != null) {
+ return false;
}
- } else {
- if (kv.getKey().startsWith(keyPrefix)) {
- return false; // we found at least one key with this vol/bucket
- // prefix.
+ }
+
+ // Check in the table
+ try (
+ TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+ keyIter = table.iterator()) {
+ KeyValue<String, OmKeyInfo> kv = keyIter.seek(bucketId);
+
+ if (kv != null) {
+ // Check the entry in db is not marked for delete. This can happen
+ // while entry is marked for delete, but it is not flushed to DB.
+ CacheValue<OmKeyInfo> cacheValue =
+ table.getCacheValue(new CacheKey(kv.getKey()));
+ if (cacheValue != null) {
+ if (kv.getKey().startsWith(bucketId)
+ && cacheValue.getCacheValue() != null) {
+ return false; // we found at least one file/dir under this
+ // bucket
+ }
+ } else {
+ if (kv.getKey().startsWith(bucketId)) {
+ return false; // we found at least one file/dir under this
+ // bucket.
+ }
+ }
}
}
}
+ } else {
Review comment:
The else branch seems to have a lot of duplication. Could we extract the bulk of the logic into a method and call the same method for both cases?
Also, would it make sense to structure the code like:
```
checkKeyTable
if (FSO bucket) {
checkDirTable
}
```
As it seems like the check in the key table is common to both?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JyotinderSingh commented on a change in pull request #3227: HDDS-6425. OmMetadataManagerImpl#isBucketEmpty does not work on FSO buckets.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833726910
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -1455,6 +1458,9 @@ public void testRenameFile() throws Exception {
assertTrue("Renamed failed: /dir/file1", getFs().exists(file1Destin));
FileStatus[] fStatus = getFs().listStatus(dirPath);
assertEquals("Renamed failed", 1, fStatus.length);
+
+ // We cannot delete a non-empty bucket.
+ getFs().delete(dirPath, true);
Review comment:
It seems like this is currently needed for the FSO variant of this test (since recursive behaviour is not being followed correctly for FSO). I will add a check for the bucket layout and delete these directories in the FSO case.
We can remove that condition once the behaviour is fixed.
Does this approach sound good?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org