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/02/17 14:06:21 UTC
[GitHub] [ozone] JyotinderSingh opened a new pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
JyotinderSingh opened a new pull request #3109:
URL: https://github.com/apache/ozone/pull/3109
## What changes were proposed in this pull request?
We are in process of deprecating `ozone.om.enable.filesystem.paths`.
We need to remove all the dependencies on the OzoneManager.getEnableFileSystemPaths() method since it depends on the `OZONE_OM_ENABLE_FILESYSTEM_PATHS` config key.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5371
## How was this patch tested?
Related Integration and unit 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r809090435
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java
##########
@@ -414,96 +411,14 @@ private OMRequest createKeyRequest(boolean isMultipartKey, int partNumber,
.setCreateKeyRequest(createKeyRequest).build();
}
- @Test
Review comment:
This test is no longer valid for LEGACY / OBJECT_STORE case, so I have moved it to the FSO variant of this test suite.
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510474
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -114,26 +114,23 @@
@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
- new Object[]{true, true, true},
- new Object[]{true, true, false},
- new Object[]{true, false, false},
- new Object[]{false, true, false},
- new Object[]{false, false, false}
+ new Object[]{true, true},
+ new Object[]{true, false},
+ new Object[]{false, false},
+ new Object[]{false, false}
Review comment:
Removed enableFileSystemPaths parameter.
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510601
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -918,10 +926,20 @@ public void testListStatusRootAndVolumeContinuation() throws IOException {
fileStatusesLimit1[fileStatusesLimit1.length - 1].getPath().toString();
FileStatus[] fileStatusesLimit2 = customListStatus(new Path("/"),
false, nextStartPath, 3);
+
// Note: at the time of writing this test, OmMetadataManagerImpl#listVolumes
// excludes startVolume (startPath) from the result. Might change.
- Assert.assertEquals(fileStatusesOver.length,
- fileStatusesLimit1.length + fileStatusesLimit2.length);
+ if (enableAcl) {
+ // 1 is added to actual number of fileStatuses due to volume created by
+ // userXXXXX in initClusterAndEnv being included in the result when ACL
+ // is enabled.
+ Assert.assertEquals(fileStatusesOver.length,
+ fileStatusesLimit1.length + fileStatusesLimit2.length + 1);
+ } else {
+ Assert.assertEquals(fileStatusesOver.length,
+ fileStatusesLimit1.length + fileStatusesLimit2.length);
+ }
Review comment:
@rakeshadr
This is a behavior change in the test, could you review 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510601
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -918,10 +926,20 @@ public void testListStatusRootAndVolumeContinuation() throws IOException {
fileStatusesLimit1[fileStatusesLimit1.length - 1].getPath().toString();
FileStatus[] fileStatusesLimit2 = customListStatus(new Path("/"),
false, nextStartPath, 3);
+
// Note: at the time of writing this test, OmMetadataManagerImpl#listVolumes
// excludes startVolume (startPath) from the result. Might change.
- Assert.assertEquals(fileStatusesOver.length,
- fileStatusesLimit1.length + fileStatusesLimit2.length);
+ if (enableAcl) {
+ // 1 is added to actual number of fileStatuses due to volume created by
+ // userXXXXX in initClusterAndEnv being included in the result when ACL
+ // is enabled.
+ Assert.assertEquals(fileStatusesOver.length,
+ fileStatusesLimit1.length + fileStatusesLimit2.length + 1);
+ } else {
+ Assert.assertEquals(fileStatusesOver.length,
+ fileStatusesLimit1.length + fileStatusesLimit2.length);
+ }
Review comment:
@rakeshadr
This is a behaviour change in the test, could you review 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510412
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemMetrics.java
##########
@@ -155,6 +153,12 @@ private void testOzoneFileCommit(TestOps op) throws Exception {
long numKeysAfterDelete = cluster
.getOzoneManager().getMetrics().getNumKeys();
Assert.assertTrue(numKeysAfterDelete >= 0);
- Assert.assertEquals(numKeysBeforeCreate, numKeysAfterDelete);
+ // The intermediate directories in the filePath are added to the directory
+ // table, and are added to the metrics as a key.
+ // When we delete the Key/File/Directory - the intermediate directories
+ // added to the directory table are cleared out using
+ // DirectoryDeletingService - which does not decrement the numKey metrics.
+ // Therefore, the numKeys metrics should be one more than the initial value.
+ Assert.assertEquals(numKeysBeforeCreate + 1, numKeysAfterDelete);
Review comment:
@rakeshadr
This looks like a behavior change in the test, could you review 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510412
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemMetrics.java
##########
@@ -155,6 +153,12 @@ private void testOzoneFileCommit(TestOps op) throws Exception {
long numKeysAfterDelete = cluster
.getOzoneManager().getMetrics().getNumKeys();
Assert.assertTrue(numKeysAfterDelete >= 0);
- Assert.assertEquals(numKeysBeforeCreate, numKeysAfterDelete);
+ // The intermediate directories in the filePath are added to the directory
+ // table, and are added to the metrics as a key.
+ // When we delete the Key/File/Directory - the intermediate directories
+ // added to the directory table are cleared out using
+ // DirectoryDeletingService - which does not decrement the numKey metrics.
+ // Therefore, the numKeys metrics should be one more than the initial value.
+ Assert.assertEquals(numKeysBeforeCreate + 1, numKeysAfterDelete);
Review comment:
@rakeshadr
This is a behavior change in the test, could you review 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510474
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -114,26 +114,23 @@
@Parameterized.Parameters
public static Collection<Object[]> data() {
return Arrays.asList(
- new Object[]{true, true, true},
- new Object[]{true, true, false},
- new Object[]{true, false, false},
- new Object[]{false, true, false},
- new Object[]{false, false, false}
+ new Object[]{true, true},
+ new Object[]{true, false},
+ new Object[]{false, false},
+ new Object[]{false, false}
Review comment:
Removed enableFileSystemPaths parameter. FSO case is covered by its respective class already.
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r811572196
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFSWithObjectStoreCreate.java
##########
@@ -293,13 +290,15 @@ public void testMPUFailDuetoDirectoryCreationBeforeComplete()
Arrays.fill(b, (byte)96);
ozoneOutputStream.write(b);
+ // Need to close ozoneOutputStream before creating directory.
+ // S3MultipartUploadCommitPartRequestWithFSO#getOpenKey makes an
+ // OMFileRequest#getParentID call - which will fail due to existing
+ // directory.
+ ozoneOutputStream.close();
+
Review comment:
we now need to close the `ozoneOutputStream` before creating new directories, I have mentioned the reason in the code comment. This is a behavior change in the test, could you please review this @rakeshadr?
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810571923
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestReadRetries.java
##########
@@ -95,19 +88,6 @@
storageContainerLocationClient;
private static final String SCM_ID = UUID.randomUUID().toString();
- private String bucketLayout;
-
- public TestReadRetries(String bucketLayout) {
- this.bucketLayout = bucketLayout;
- }
-
- @Parameterized.Parameters
- public static Collection<Object[]> data() {
- return Arrays.asList(
- new Object[]{OMConfigKeys.OZONE_DEFAULT_BUCKET_LAYOUT_DEFAULT},
- new Object[]{OMConfigKeys.
- OZONE_BUCKET_LAYOUT_FILE_SYSTEM_OPTIMIZED});
- }
Review comment:
This is a file system related test, which is no longer valid for bucket layouts other than FSO since we are in process of removing the `OZONE_OM_ENABLE_FILESYSTEM_PATHS` configuration key.
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r810510553
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
##########
@@ -897,9 +891,23 @@ public void testListStatusRootAndVolumeContinuation() throws IOException {
// numEntries > 5
FileStatus[] fileStatusesOver = customListStatus(new Path("/"),
false, "", 8);
- // There are only 5 volumes
+
+ // There are only 5 volumes created by current user.
// Default volume "s3v" is created during startup.
- Assert.assertEquals(5 + 1, fileStatusesOver.length);
+ //
+ // When ACL is enabled, listStatus root will see a 7th volume created by
+ // userXXXXX as the result of createVolumeAndBucket in initClusterAndEnv.
+ // This is due to the difference in behavior in listVolumesByUser depending
+ // on whether ACL is enabled or not:
+ // 1. when ACL is disabled, listVolumesByUser would only return volumes
+ // OWNED by the current user (volume owner is the current user);
+ // 2. when ACL is enabled, it would return all the volumes that the current
+ // user has LIST permission to, regardless of the volume owner field.
+ if (enableAcl) {
+ Assert.assertEquals(5 + 1 + 1, fileStatusesOver.length);
+ } else {
+ Assert.assertEquals(5 + 1, fileStatusesOver.length);
+ }
Review comment:
@rakeshadr
This is a behavior change in the test, could you review 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] JyotinderSingh commented on a change in pull request #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r809091618
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java
##########
@@ -414,96 +411,14 @@ private OMRequest createKeyRequest(boolean isMultipartKey, int partNumber,
.setCreateKeyRequest(createKeyRequest).build();
}
- @Test
- public void testKeyCreateWithFileSystemPathsEnabled() throws Exception {
Review comment:
This test is no longer valid for LEGACY / OBJECT_STORE case, so I have moved it to the FSO variant of this test suite.
--
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 #3109: HDDS-5371. [FSO] Remove all the dependencies on OzoneManager#getEnableFileSystemPaths.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on a change in pull request #3109:
URL: https://github.com/apache/ozone/pull/3109#discussion_r809090435
##########
File path: hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java
##########
@@ -414,96 +411,14 @@ private OMRequest createKeyRequest(boolean isMultipartKey, int partNumber,
.setCreateKeyRequest(createKeyRequest).build();
}
- @Test
Review comment:
This test is no longer valid for LEGACY / OBJECT_STORE case.
--
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