You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "kaijchen (via GitHub)" <gi...@apache.org> on 2023/02/02 07:57:40 UTC
[GitHub] [ozone] kaijchen opened a new pull request, #4236: Fix false positive in KeyManagerImpl#createFakeDirIfShould()
kaijchen opened a new pull request, #4236:
URL: https://github.com/apache/ozone/pull/4236
## What changes were proposed in this pull request?
`RocksIterator#seek()` may position at the first key in the source that at or **past target**,
which results false positive in KeyManagerImpl#createFakeDirIfShould().
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7871
## How was this patch tested?
TestKeyManagerImpl#testGetFileStatusWithFakeDirHDDS7871
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1095857040
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,51 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirForHDDS7871() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/foo1"
+ // 2. "volume1/bucket2/foo2/bar2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName1)));
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET2_NAME, keyName2)));
+
+ // get a non-existing "foo2" from bucket1, RocksIter#seek() will find the
+ // 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
Review Comment:
```java
// 1. "volume1/bucket1/foo1"
// seek -> "volume1/bucket1/foo2/"
// pos -> 2. "volume1/bucket2/foo2/bar2"
```
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099088043
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Thanks for the upgrading to Junit-5 @kaijchen.
Can you please remove Junit-4 import for this class? And change `Assertions`.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099597109
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
It's not because of amount of work. It's because it makes commit history messy.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1095845345
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,51 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirForHDDS7871() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/foo1"
+ // 2. "volume1/bucket2/foo2/bar2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName1)));
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET2_NAME, keyName2)));
+
+ // get a non-existing "foo2" from bucket1, RocksIter#seek() will find the
+ // 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
Review Comment:
> `RocksIterator#seek()` may position at the key **past the target**
Yes, here seeking `volume1/bucket1/foo2/`, it will position at `volume1/bucket2/foo2/bar2`.
--
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] smengcl commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1096209540
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,51 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirForHDDS7871() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/foo1"
+ // 2. "volume1/bucket2/foo2/bar2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName1)));
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET2_NAME, keyName2)));
+
+ // get a non-existing "foo2" from bucket1, RocksIter#seek() will find the
+ // 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
+ keyArgs = createBuilder(BUCKET_NAME).setKeyName(dirName).build();
+ OmKeyArgs finalKeyArgs = keyArgs;
+ Assert.assertThrows(OMException.class, () -> keyManager.getFileStatus(
+ finalKeyArgs));
+
Review Comment:
nit: check the ResultCode as well (`FILE_NOT_FOUND`)
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099620162
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
It turns out there are some unexpected test logic change required in #4257.
See the `@Rule ExpectedException` part in this commit: https://github.com/apache/ozone/pull/4257/commits/95a11d6a4c87cd772571e33f3493d200825975ef
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099088043
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Thanks for the upgrading to Junit-5 @kaijchen.
Can you please remove Junit-4 import for this class? And change `Assertions`.
Sorry for the pushing on this.
--
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] smengcl commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099296435
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Hi @adoroszlai , I agree that generally in a bug fix we should not do refactoring (because of backportability concern). Though in this case I'd say it is alright as the "refactoring" is in the UT.
I'm fine either way. :)
--
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] kaijchen commented on pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #4236:
URL: https://github.com/apache/ozone/pull/4236#issuecomment-1422417748
Updated this PR after migrating the test to JUnit5.
@hemantk-12 @adoroszlai please take another look, thanks.
--
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] kaijchen commented on pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #4236:
URL: https://github.com/apache/ozone/pull/4236#issuecomment-1420131432
Thanks @hemantk-12 for the suggestions. The extra checks are added.
However, the current test class is still in JUnit 4, so let's do not change it to parameterized now.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1098127895
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
This test class is still in JUnit 4, let's not change it now?
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099587089
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Not going to stretch it or argue any further. But Changing `@Rule` is one line change and `Assert.assertXX` is also one or two in IDE if you do a static import.
You already have a +1. So feel free to merge.
--
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] adoroszlai commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099191353
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
@hemantk-12 let's not mix refactor and bugfix
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099568040
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Hi @hemantk-12, there is more than just changing imports.
`@Rule` from JUnit 4 should also be replaced (~10 lines of change).
And `Assert.assertXXX` need to be replaced by `assertXXX` (~100 lines of change).
I can submit another PR right after this to do the migration.
--
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 diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1095785154
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,51 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirForHDDS7871() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/foo1"
+ // 2. "volume1/bucket2/foo2/bar2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName1)));
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET2_NAME, keyName2)));
+
+ // get a non-existing "foo2" from bucket1, RocksIter#seek() will find the
+ // 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
Review Comment:
> HDDS-7871: RocksIterator#seek() may position at the key past the target, we should check the full dbKeyName.
> RocksIter#seek() will find the 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
But here seek(volume1/bucket1/foo2/bar2) is seeking to "volume1/bucket2/foo2/bar2" which means it can position it behind the target too ? 😮
--
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] adoroszlai commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099778086
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
> But if you look at the original patch, these are the new tests which were added and I asked them to be in Junit-5.
@hemantk-12 Sorry for saying so, but this caused the "problem" (the mix of refactoring and bugfix).
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1097992377
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
```suggestion
private static Stream<Arguments> getFileStatusWithFakeDirFalsePositiveScenarios() {
String dir0 = "dir0";
String dir1 = "dir1";
String dir2 = "dir2";
String dir3 = "dir3";
String keyName1 = dir1 + OZONE_URI_DELIMITER + "file1";
String keyName2 = dir2 + OZONE_URI_DELIMITER + "file2";
return Stream.of(
Arguments.of("Case 1: volume1/bucket1/dir1/file1", // You can explain more here about the test case like input and out
BUCKET_NAME,
keyName1,
Collections.singletonList(dir1),
Arrays.asList(dir0, dir2, dir3)
),
Arguments.of("Case 2: volume1/bucket2/dir2/file2", // You can explain more here about the test case like input and out
BUCKET_2_NAME,
keyName2,
Collections.singletonList(dir2),
Arrays.asList(dir0, dir1, dir3)
)
);
}
@ParameterizedTest(name = "{0}")
@MethodSource("getFileStatusWithFakeDirFalsePositiveScenarios")
public void testGetFileStatusWithFakeDirFalsePositive(String description,
String bucketName,
String keyName,
List<String> expectedDirs,
List<String> notExpectedDirs)
throws IOException {
createFile(bucketName, keyName);
Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
.get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName)));
// Assert volumeName, bucketName and keyName as well;
for (String notExpectedDir: notExpectedDirs) {
assertFileNotFound(BUCKET_NAME, notExpectedDir);
}
for (String expectedDir: expectedDirs) {
assertIsDirectory(BUCKET_NAME, expectedDir);
}
}
```
I think it would be easier to read and understand the test scenarios if you change the test to parameterize in Junit-5. And it can be a single test instead of `testGetFileStatusWithFakeDirFalsePositive` and `testGetFileStatusWithFakeDirFalseNegative` as separate tests.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
+ String dir0 = "dir0";
+ String dir1 = "dir1";
+ String dir2 = "dir2";
+ String dir3 = "dir3";
+ String keyName1 = dir1 + OZONE_URI_DELIMITER + "file1";
+ String keyName2 = dir2 + OZONE_URI_DELIMITER + "file2";
+
+ // create a key "dir1/file1" in bucket1
+ createFile(BUCKET_NAME, keyName1);
+ // create a key "dir2/file2" in bucket2
+ createFile(BUCKET2_NAME, keyName2);
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/dir1/file1"
+ // 2. "volume1/bucket2/dir2/file2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
Review Comment:
I'll suggestion to asset volume, bucket and key names as well.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1587,6 +1663,31 @@ private List<String> createFiles(String parent,
return keyNames;
}
+ private void createFile(String bucketName, String keyName)
+ throws IOException {
+ OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+ }
+
+ private void assertFileNotFound(String bucketName, String keyName)
+ throws IOException {
+ OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
+ OMException ex = Assert.assertThrows(OMException.class,
+ () -> keyManager.getFileStatus(keyArgs));
+ Assert.assertEquals(OMException.ResultCodes.FILE_NOT_FOUND, ex.getResult());
+ }
+
+ private void assertIsDirectory(String bucketName, String keyName)
+ throws IOException {
+ OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build();
+ OzoneFileStatus ozoneFileStatus = keyManager.getFileStatus(keyArgs);
+ Assert.assertEquals(keyName, ozoneFileStatus.getKeyInfo().getFileName());
Review Comment:
We should assert `volumeName` and `bucketName`.
Run the test few times and make sure that it passes all the time.
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1098172864
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Junit-5 is already a dependency. You just have to change the import to junit-5. No change would be needed in the existing tests.
--
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] adoroszlai commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099778086
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
> But if you look at the original patch, these are the new tests which were added and I asked them to be in Junit-5.
@hemantk-12 Sorry for saying that, but this caused the problem.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
> But if you look at the original patch, these are the new tests which were added and I asked them to be in Junit-5.
@hemantk-12 Sorry for saying so, but this caused the problem.
--
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] adoroszlai merged pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4236:
URL: https://github.com/apache/ozone/pull/4236
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1098224257
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Thanks, I have updated the 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] adoroszlai commented on pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4236:
URL: https://github.com/apache/ozone/pull/4236#issuecomment-1419724091
@hemantk-12 can you please take another look?
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1096286520
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,53 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
Review Comment:
nit: These lines (1373-1377) are repetitive as line from 1366-1370. May be create a helper function or use existing one if there is any.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1178,21 +1178,18 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
String keyName, BucketLayout layout) throws IOException {
OmKeyInfo fakeDirKeyInfo = null;
String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
- String fileKeyBytes = metadataManager.getOzoneKey(volume, bucket, keyName);
+ String targetKey = OzoneFSUtils.addTrailingSlashIfNeeded(
+ metadataManager.getOzoneKey(volume, bucket, keyName));
try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyTblItr = metadataManager.getKeyTable(layout).iterator()) {
- Table.KeyValue<String, OmKeyInfo> keyValue =
- keyTblItr
- .seek(OzoneFSUtils.addTrailingSlashIfNeeded(fileKeyBytes));
-
- if (keyValue != null) {
- Path fullPath = Paths.get(keyValue.getValue().getKeyName());
- Path subPath = Paths.get(dirKey);
- OmKeyInfo omKeyInfo = keyValue.getValue();
- if (fullPath.startsWith(subPath)) {
- // create fake directory
- fakeDirKeyInfo = createDirectoryKey(omKeyInfo, dirKey);
- }
+ Table.KeyValue<String, OmKeyInfo> keyValue = keyTblItr.seek(targetKey);
+
+ // HDDS-7871: RocksIterator#seek() may position at the key
Review Comment:
Is it because seek may position past target or bloom filter or when item is not in the table?
As per my understanding it seems like [false positive because of bloom filter](https://github.com/facebook/rocksdb/wiki/Prefix-Seek). Please correct me if I'm wrong.
When I was testing it, my test was failure for following scenario.
Items in the table was:
| KeyInfoTable |
|-------------------|
| /vol1/bucket1/key1 |
| /vol1/bucket1/key2 |
| /vol1/bucket1/key3 |
| /vol1/bucket1/key4 |
| /vol1/bucket1/key5 |
Was looking for `/vol2/bucket2/key1` and getting false positive.
May point is if it is because of bloom filter and seek is pointing to wrong item, we might miss the "fakeDir".
Eg. You have following table
| KeyInfoTable |
|-----------------------|
| /vol1/bucket1/dir1/key1 |
| /vol1/bucket1/dir1/key2 |
| /vol1/bucket1/dir1/key3 |
| /vol1/bucket2/dir1/key1 |
| /vol1/bucket2/dir1/key2 |
and now you are looking for `/vol1/bucket2/dir1` but seek is pointing to ` /vol1/bucket1/dir1/key1`. So it will be a miss all the time now.
--
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 diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1095785154
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,51 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirForHDDS7871() throws IOException {
+ String dirName = "foo2";
+ String fileName = "bar2";
+ String keyName1 = "foo1";
+ // keyName2 = "foo2/bar2"
+ String keyName2 = dirName + OZONE_URI_DELIMITER + fileName;
+ OzoneFileStatus ozoneFileStatus;
+
+ // create a key "foo1" in bucket1
+ OmKeyArgs keyArgs = createBuilder(BUCKET_NAME).setKeyName(keyName1).build();
+ OpenKeySession keySession = writeClient.openKey(keyArgs);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // create a key "foo2/bar2" in bucket2
+ keyArgs = createBuilder(BUCKET2_NAME).setKeyName(keyName2).build();
+ keySession = writeClient.createFile(keyArgs, true, true);
+ keyArgs.setLocationInfoList(
+ keySession.getKeyInfo().getLatestVersionLocations().getLocationList());
+ writeClient.commitKey(keyArgs, keySession.getId());
+
+ // Verify the following dbKeys in key table:
+ // 1. "volume1/bucket1/foo1"
+ // 2. "volume1/bucket2/foo2/bar2"
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET_NAME, keyName1)));
+ Assert.assertNotNull(metadataManager.getKeyTable(getDefaultBucketLayout())
+ .get(metadataManager.getOzoneKey(VOLUME_NAME, BUCKET2_NAME, keyName2)));
+
+ // get a non-existing "foo2" from bucket1, RocksIter#seek() will find the
+ // 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
Review Comment:
> HDDS-7871: RocksIterator#seek() may position at the key past the target, we should check the full dbKeyName.
> RocksIter#seek() will find the 2nd dbKey "volume1/bucket2/foo2/bar2", which is not belong to bucket1.
But here seek(volume1/bucket1/foo2/bar2) is seeking to "volume1/bucket2/foo2/bar2" which means it can position it behind the target too ?
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1096478075
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1178,21 +1178,18 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
String keyName, BucketLayout layout) throws IOException {
OmKeyInfo fakeDirKeyInfo = null;
String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
- String fileKeyBytes = metadataManager.getOzoneKey(volume, bucket, keyName);
+ String targetKey = OzoneFSUtils.addTrailingSlashIfNeeded(
+ metadataManager.getOzoneKey(volume, bucket, keyName));
try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyTblItr = metadataManager.getKeyTable(layout).iterator()) {
- Table.KeyValue<String, OmKeyInfo> keyValue =
- keyTblItr
- .seek(OzoneFSUtils.addTrailingSlashIfNeeded(fileKeyBytes));
-
- if (keyValue != null) {
- Path fullPath = Paths.get(keyValue.getValue().getKeyName());
- Path subPath = Paths.get(dirKey);
- OmKeyInfo omKeyInfo = keyValue.getValue();
- if (fullPath.startsWith(subPath)) {
- // create fake directory
- fakeDirKeyInfo = createDirectoryKey(omKeyInfo, dirKey);
- }
+ Table.KeyValue<String, OmKeyInfo> keyValue = keyTblItr.seek(targetKey);
+
+ // HDDS-7871: RocksIterator#seek() may position at the key
Review Comment:
Thanks for the info. I will look into this scenario also.
--
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] adoroszlai commented on pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4236:
URL: https://github.com/apache/ozone/pull/4236#issuecomment-1422798326
Thanks @kaijchen for updating the patch, @hemantk-12, @sadanand48, @smengcl for the review.
--
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] hemantk-12 commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099365528
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Hi @adoroszlai,
As @smengcl , said I agree that it is not a good idea to mix bug fix and refactoring. But if you look at the original patch, these are the new tests which were added and I asked them to be in Junit-5. There was no change or refactoring to the existing code. Yes, we needed to change some imports, but I think that's ok and not a refactoring.
Now we have duplicate imports [BeforeClass/BeforeAll](https://github.com/apache/ozone/pull/4236/files#diff-4275d85bf442cb352596248ced7651fcd5e3b09876bb6d2d80464cdf628a23fdR184-R185), [AfterClass/AfterAll](https://github.com/apache/ozone/pull/4236/files#diff-4275d85bf442cb352596248ced7651fcd5e3b09876bb6d2d80464cdf628a23fdR244-R245) and [Test](https://github.com/apache/ozone/pull/4236/files#diff-4275d85bf442cb352596248ced7651fcd5e3b09876bb6d2d80464cdf628a23fdR126).
```
@BeforeClass
@BeforeAll
```
and
```
@AfterClass
@AfterAll
```
If you think that having duplicate imports (the current situation) and clean up is not needed is ok, I'm fine you merging it.
Also if we don't do these type of update/upgrade one by one, we will never upgrade existing stuff unless we are forced which is rare in my opinion.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099568040
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Hi @hemantk-12, there is more than just changing imports.
`@Rule` from JUnit 4 should also be replaced (although it's just around 10 lines of change).
I can submit another PR right after this to do the migration.
--
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] adoroszlai commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099779686
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
> if we don't do these type of update/upgrade one by one, we will never upgrade existing stuff unless we are forced which is rare in my opinion.
If you look at Ozone's history, it was @kaijchen who migrated lots of tests to JUnit5.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1099787882
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
Hi @adoroszlai, I think the best way is to merge #4257 first. So there won't be any JUnit5 related changes in this PR.
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1098127895
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java:
##########
@@ -1351,6 +1353,80 @@ public void testGetFileStatusWithFakeDir() throws IOException {
Assert.assertTrue(ozoneFileStatus.isFile());
}
+ @Test
+ public void testGetFileStatusWithFakeDirFalsePositive() throws IOException {
Review Comment:
This test class is still in Junit 4, let's not change it now?
--
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] kaijchen commented on pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #4236:
URL: https://github.com/apache/ozone/pull/4236#issuecomment-1413329606
Cc @hemantk-12 @xichen01
--
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] kaijchen commented on a diff in pull request #4236: HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould()
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #4236:
URL: https://github.com/apache/ozone/pull/4236#discussion_r1096485674
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1178,21 +1178,18 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
String keyName, BucketLayout layout) throws IOException {
OmKeyInfo fakeDirKeyInfo = null;
String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
- String fileKeyBytes = metadataManager.getOzoneKey(volume, bucket, keyName);
+ String targetKey = OzoneFSUtils.addTrailingSlashIfNeeded(
+ metadataManager.getOzoneKey(volume, bucket, keyName));
try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
keyTblItr = metadataManager.getKeyTable(layout).iterator()) {
- Table.KeyValue<String, OmKeyInfo> keyValue =
- keyTblItr
- .seek(OzoneFSUtils.addTrailingSlashIfNeeded(fileKeyBytes));
-
- if (keyValue != null) {
- Path fullPath = Paths.get(keyValue.getValue().getKeyName());
- Path subPath = Paths.get(dirKey);
- OmKeyInfo omKeyInfo = keyValue.getValue();
- if (fullPath.startsWith(subPath)) {
- // create fake directory
- fakeDirKeyInfo = createDirectoryKey(omKeyInfo, dirKey);
- }
+ Table.KeyValue<String, OmKeyInfo> keyValue = keyTblItr.seek(targetKey);
+
+ // HDDS-7871: RocksIterator#seek() may position at the key
Review Comment:
While I'm still reading the RocksDB bloom filter docs, I have added more test cases to cover your scenarios.
> Was looking for `/vol2/bucket2/key1` and getting false positive.
Looking for `/vol1/bucket2/dir3/` is added in my false positive test. Is it equivalent?
> and now you are looking for `/vol1/bucket2/dir1` but seek is pointing to ` /vol1/bucket1/dir1/key1`. So it will be a miss all the time now.
TestFalseNegative is added, but I'm not sure why it might happen.
--
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