chihsuan commented on code in PR #11479:
URL: https://github.com/apache/ozone/pull/11479#discussion_r4301178188
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -341,6 +341,15 @@ void checkBucketAccess(String volumeName, String
bucketName)
OzoneBucket getBucketDetails(String volumeName, String bucketName)
throws IOException;
+ /**
+ * Returns Bucket Information.
+ * @param bucketName Name of the Bucket.
+ * @return OzoneBucket
+ * @throws IOException
+ */
+ OzoneBucket getS3BucketDetails(String bucketName)
Review Comment:
Just a heads-up, #11458 also adds `getS3BucketDetails`. If it merges first,
could we reuse its implementation and drop the duplicate here?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneManagerVersion.java:
##########
@@ -73,6 +73,8 @@ public enum OzoneManagerVersion implements ComponentVersion {
+ "server-side, so file system clients no longer need the "
+ "client-side InfoBucket layout check for getFileChecksum"),
ZDU(100, "OzoneManager version that supports zero downtime upgrade"),
+ S3_BUCKET_INFO_WITH_VOLUME_CONTEXT(101,
Review Comment:
A gentle reminder, master already took 101 in the meantime. Might be worth
double-checking the next free number before merging.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -199,6 +199,7 @@ message OMRequest {
optional LayoutVersion layoutVersion = 6;
optional ReadConsistencyHint readConsistencyHint = 7;
+ optional bool assumeS3Context = 8;
Review Comment:
Heads-up rather than a change request: #11458 puts `assumeS3Context` on
`InfoBucketRequest` instead. I left a comment there to align on one place,
would love your thoughts too.
##########
hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-sts-multitenant.robot:
##########
@@ -269,7 +269,7 @@ Verify Limited-Scoped Token Accesses
Get Object Should Fail ${TENANT_ONE_LINKED_BUCKET}
${ANOTHER_BUCKET_TESTFILE} AccessDenied
Put Object Should Fail ${TENANT_ONE_LINKED_BUCKET}
${ANOTHER_BUCKET_TESTFILE} AccessDenied
Get Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET}
${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
- Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET}
${ANOTHER_BUCKET_TESTFILE} AccessDenied
+ Put Object Should Fail ${TENANT_TWO_ANOTHER_BUCKET}
${ANOTHER_BUCKET_TESTFILE} NoSuchBucket
Review Comment:
Just curious, is the switch to NoSuchBucket here intentional? I found it
comes from skipping the volume check with `getS3VolumeContext(true)`, while
#11458 keeps that check and still returns AccessDenied. Either way works for
me, as long as we pick the same one.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3RequestContext.java:
##########
@@ -59,13 +59,20 @@ OzoneVolume getVolume() throws IOException {
OzoneBucket getBucket(String name) throws IOException {
if (bucket == null) {
- bucket = getVolume().getBucket(name);
+ bucket = endpoint.getClientProtocol().getS3BucketDetails(name);
} else {
Preconditions.assertEquals(bucket.getName(), name, "Multiple buckets in
one request are not supported");
}
return bucket;
}
+ /**
+ * Returns the S3 volume name, reusing the bucket already looked up in this
request to avoid another call.
+ */
+ String getVolumeName() throws IOException {
Review Comment:
nit: Could we verify `getS3Volume()` isn't called once the bucket is cached?
Reverting this still passes the 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: [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]
