errose28 commented on code in PR #4186:
URL: https://github.com/apache/ozone/pull/4186#discussion_r1084409465
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -245,6 +257,8 @@ public Response put(
throw newError(S3ErrorTable.ACCESS_DENIED, keyPath, ex);
} else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) {
throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
+ } else if (ResultCodes.FILE_ALREADY_EXISTS == ex.getResult()) {
Review Comment:
nit. to match formatting
```suggestion
} else if (ex.getResult() == ResultCodes.FILE_ALREADY_EXISTS) {
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -196,6 +199,15 @@ public Response put(
// Normal put object
OzoneBucket bucket = volume.getBucket(bucketName);
+ if (length == 0 &&
+ OZONE_S3G_FSO_DIRECTORY_CREATION_ENABLED &&
Review Comment:
Similar to above, this value needs to be checked for what the user set by
getting the key from a `ConfigurationSource` or `OzoneConfiguration` object.
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java:
##########
@@ -288,4 +297,80 @@ public void testEmptyStorageType() throws IOException,
OS3Exception {
//default type is set
Assert.assertEquals(ReplicationType.RATIS, key.getReplicationType());
}
+
+ @Test
+ public void testDirectoryCreation() throws IOException,
Review Comment:
These tests have significantly more mocking than others in this class. Can
we make them more similar to the other tests with less mocking using the
`clientStub` and `objectEndpoint` fields that already exist?
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -245,6 +257,8 @@ public Response put(
throw newError(S3ErrorTable.ACCESS_DENIED, keyPath, ex);
} else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) {
throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
+ } else if (ResultCodes.FILE_ALREADY_EXISTS == ex.getResult()) {
+ throw newError(S3ErrorTable.INVALID_REQUEST, keyPath, ex);
Review Comment:
I think this case warrants its own entry in the `S3ErrorTable` so that the
end user with the s3 client sees a message explaining what happened. This will
be very confusing otherwise.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/S3GatewayConfigKeys.java:
##########
@@ -63,7 +63,7 @@ public final class S3GatewayConfigKeys {
"ozone.s3g.kerberos.keytab.file";
public static final String OZONE_S3G_KERBEROS_PRINCIPAL_KEY =
"ozone.s3g.kerberos.principal";
-
+ public static final boolean OZONE_S3G_FSO_DIRECTORY_CREATION_ENABLED = true;
Review Comment:
This also needs a constant for the config key the user would actually add.
Like `OZONE_S3G_KERBEROS_PRINCIPAL_KEY` above, for example.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/S3GatewayConfigKeys.java:
##########
@@ -63,7 +63,7 @@ public final class S3GatewayConfigKeys {
"ozone.s3g.kerberos.keytab.file";
public static final String OZONE_S3G_KERBEROS_PRINCIPAL_KEY =
"ozone.s3g.kerberos.principal";
-
+ public static final boolean OZONE_S3G_FSO_DIRECTORY_CREATION_ENABLED = true;
Review Comment:
Also let's put a block comment here to explain what this config means. I
filed HDDS-7824 for a more widespread user facing doc update.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
