[GitHub] [ozone] errose28 commented on a diff in pull request #4186: HDDS-7594. [FSO] Folders created through S3G are created on file system as "files".

["errose28 (via GitHub)" <gi...@apache.org>]

errose28 commented on code in PR #4186:
URL: https://github.com/apache/ozone/pull/4186#discussion_r1095149611


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/S3GatewayConfigKeys.java:
##########
@@ -63,7 +63,12 @@ public final class S3GatewayConfigKeys {
       "ozone.s3g.kerberos.keytab.file";
   public static final String OZONE_S3G_KERBEROS_PRINCIPAL_KEY =
       "ozone.s3g.kerberos.principal";
-
+  /**
+   * Configuration key that enables creation of directory instead of 0 byte
+   * file if bucket layout is FSO. Default value is <code>true</code>
+   */
+  public static final String OZONE_S3G_FSO_DIRECTORY_CREATION_ENABLED =
+      "ozone.s3g.fso.directory.creation";

Review Comment:
   Usually when things have a default value we make a constant for that too. An example from farther up in this file:
   ```
     public static final String OZONE_S3G_CLIENT_BUFFER_SIZE_KEY =
         "ozone.s3g.client.buffer.size";
     public static final String OZONE_S3G_CLIENT_BUFFER_SIZE_DEFAULT =
         "4KB";
   
   ```



##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java:
##########
@@ -122,6 +122,9 @@ private S3ErrorTable() {
       "NotImplemented", "This part of feature is not implemented yet.",
       HTTP_NOT_IMPLEMENTED);
 
+  public static final OS3Exception NO_OVERWRITE = new OS3Exception(
+      "Conflict", "Cannot overwrite file with directory", HTTP_CONFLICT);

Review Comment:
   It looks like there is no failure when writing a directory when one already exists. If it is just a no-op then this message is accurate because the no-overwrite case will only happen when a file is already there. However it may be good to manually verify that doing directory create from s3g with this PR's changes when a directory already exists there doesn't result in 500 or other obscure error.



##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java:
##########
@@ -288,4 +298,82 @@ public void testEmptyStorageType() throws IOException, OS3Exception {
     //default type is set
     Assert.assertEquals(ReplicationType.RATIS, key.getReplicationType());
   }
+
+  @Test
+  public void testDirectoryCreation() throws IOException,
+      OS3Exception {
+    // GIVEN
+    final String bucketname = "bucketName";

Review Comment:
   Can we use the `BucketName` field in this class instead? Having a local variable `bucketname` and class level `bucketName` is confusing.



##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java:
##########
@@ -288,4 +298,82 @@ public void testEmptyStorageType() throws IOException, OS3Exception {
     //default type is set
     Assert.assertEquals(ReplicationType.RATIS, key.getReplicationType());
   }
+
+  @Test
+  public void testDirectoryCreation() throws IOException,
+      OS3Exception {
+    // GIVEN
+    final String bucketname = "bucketName";
+    final String path = "dir";
+    final long length = 0L;
+    final int partNumber = 0;
+    final String uploadId = "";
+    final InputStream body = null;
+    final HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+    final ObjectEndpoint objEndpoint = new ObjectEndpoint();
+    objEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+    objEndpoint.setHeaders(headers);
+    final OzoneClient client = Mockito.mock(OzoneClient.class);
+    objEndpoint.setClient(client);
+    final ObjectStore objectStore = Mockito.mock(ObjectStore.class);
+    final OzoneVolume volume = Mockito.mock(OzoneVolume.class);
+    final OzoneBucket bucket = Mockito.mock(OzoneBucket.class);
+    final ClientProtocol protocol = Mockito.mock(ClientProtocol.class);
+
+    // WHEN
+    when(headers.getHeaderString(any())).thenReturn("");
+    when(client.getObjectStore()).thenReturn(objectStore);
+    when(client.getObjectStore().getS3Volume()).thenReturn(volume);
+    when(volume.getBucket(bucketname)).thenReturn(bucket);
+    when(bucket.getBucketLayout())
+        .thenReturn(BucketLayout.FILE_SYSTEM_OPTIMIZED);
+    when(client.getProxy()).thenReturn(protocol);
+    final Response response = objEndpoint.put(bucketname, path, length,
+        partNumber, uploadId, body);
+
+    // THEN
+    Assertions.assertEquals(HttpStatus.SC_OK, response.getStatus());
+    Mockito.verify(protocol).createDirectory(any(), any(), any());

Review Comment:
   We can verify the bucket name, path name, and invocation count explicitly.
   ```suggestion
       Mockito.verify(protocol, times(1)).createDirectory(any(),
             ArgumentMatchers.eq(bucketName),
             ArgumentMatchers.eq(path));
   
   ```



##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java:
##########
@@ -288,4 +298,82 @@ public void testEmptyStorageType() throws IOException, OS3Exception {
     //default type is set
     Assert.assertEquals(ReplicationType.RATIS, key.getReplicationType());
   }
+
+  @Test
+  public void testDirectoryCreation() throws IOException,
+      OS3Exception {
+    // GIVEN
+    final String bucketname = "bucketName";
+    final String path = "dir";
+    final long length = 0L;
+    final int partNumber = 0;
+    final String uploadId = "";
+    final InputStream body = null;
+    final HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+    final ObjectEndpoint objEndpoint = new ObjectEndpoint();
+    objEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+    objEndpoint.setHeaders(headers);
+    final OzoneClient client = Mockito.mock(OzoneClient.class);
+    objEndpoint.setClient(client);
+    final ObjectStore objectStore = Mockito.mock(ObjectStore.class);
+    final OzoneVolume volume = Mockito.mock(OzoneVolume.class);
+    final OzoneBucket bucket = Mockito.mock(OzoneBucket.class);
+    final ClientProtocol protocol = Mockito.mock(ClientProtocol.class);
+
+    // WHEN
+    when(headers.getHeaderString(any())).thenReturn("");
+    when(client.getObjectStore()).thenReturn(objectStore);
+    when(client.getObjectStore().getS3Volume()).thenReturn(volume);
+    when(volume.getBucket(bucketname)).thenReturn(bucket);
+    when(bucket.getBucketLayout())
+        .thenReturn(BucketLayout.FILE_SYSTEM_OPTIMIZED);
+    when(client.getProxy()).thenReturn(protocol);
+    final Response response = objEndpoint.put(bucketname, path, length,
+        partNumber, uploadId, body);
+
+    // THEN
+    Assertions.assertEquals(HttpStatus.SC_OK, response.getStatus());
+    Mockito.verify(protocol).createDirectory(any(), any(), any());
+  }
+
+  @Test
+  public void testDirectoryCreationOverFile() throws IOException {
+    // GIVEN
+    final String bucketname = "bucketName";
+    final String path = "key";
+    final long length = 0L;
+    final int partNumber = 0;
+    final String uploadId = "";
+    final ByteArrayInputStream body =
+        new ByteArrayInputStream("content".getBytes(UTF_8));
+    final HttpHeaders headers = Mockito.mock(HttpHeaders.class);
+    final ObjectEndpoint objEndpoint = new ObjectEndpoint();
+    objEndpoint.setOzoneConfiguration(new OzoneConfiguration());
+    objEndpoint.setHeaders(headers);
+    final OzoneClient client = Mockito.mock(OzoneClient.class);
+    objEndpoint.setClient(client);
+    final ObjectStore objectStore = Mockito.mock(ObjectStore.class);
+    final OzoneVolume volume = Mockito.mock(OzoneVolume.class);
+    final OzoneBucket bucket = Mockito.mock(OzoneBucket.class);
+    final ClientProtocol protocol = Mockito.mock(ClientProtocol.class);
+
+    // WHEN
+    when(client.getObjectStore()).thenReturn(objectStore);
+    when(client.getObjectStore().getS3Volume()).thenReturn(volume);
+    when(volume.getBucket(bucketname)).thenReturn(bucket);
+    when(bucket.getBucketLayout())
+        .thenReturn(BucketLayout.FILE_SYSTEM_OPTIMIZED);
+    when(client.getProxy()).thenReturn(protocol);
+    doThrow(new OMException(OMException.ResultCodes.FILE_ALREADY_EXISTS))
+        .when(protocol)
+        .createDirectory(any(), any(), any());
+
+    // THEN
+    final OS3Exception exception = Assertions.assertThrows(OS3Exception.class,
+        () -> objEndpoint
+            .put(bucketname, path, length, partNumber, uploadId, body));
+    Assertions.assertEquals("Conflict", exception.getCode());
+    Assertions.assertEquals(409, exception.getHttpCode());
+    Mockito.verify(protocol, times(1)).createDirectory(any(), any(), any());

Review Comment:
   Since the setup for these two new test is pretty much identical, these highlighted lines can actually be added to the end of the first test to cover both cases. Invocation count on on `Mockito#verify` would just need to be increased to 2.



-- 
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