anuragp010 commented on code in PR #11626:
URL: https://github.com/apache/ozone/pull/11626#discussion_r4357113855
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java:
##########
@@ -90,12 +92,23 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
keyPath = validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(),
keyPath, getBucketLayout());
+ final OmBucketInfo bucketInfo = ozoneManager
+ .getBucketInfo(keyArgs.getVolumeName(), keyArgs.getBucketName());
+
KeyArgs.Builder newKeyArgs = keyArgs.toBuilder()
.setMultipartUploadID(
OMMultipartUploadUtils.getMultipartUploadId())
.setModificationTime(Time.now())
.setKeyName(keyPath);
+ // Stamp the policy only once finalized: an OM without storage policy
support ignores the field, so stamping it
+ // earlier would make old and new OMs apply this request differently.
Clearing also drops a client-sent value.
+ if
(ozoneManager.getVersionManager().isAllowed(OMLayoutFeature.STORAGE_POLICY_SUPPORT))
{
+
newKeyArgs.setStoragePolicy(OzoneStoragePolicy.toProto(getStoragePolicy(bucketInfo,
keyArgs)));
+ } else {
+ newKeyArgs.clearStoragePolicy();
+ }
Review Comment:
@Gargi-jais11 , as discussed, thanks for raising this point. As of now, the
expectation was that the client would send the default policy when the user
doesn't pass any storagePolicy. So in the case of `initiateMultipartUpload`, it
was assumed the default Storage policy, `WARM`, would be sent as part of the
request to the OM if the user didn't pass a storagePolicy.
But since we're sending the default storage policy, there's no way for the
OM to differentiate wether a client sent the request explicitly or the default
was added by the client side code.
1. As pointed out by @Gargi-jais11 should we have a `OzoneManagerVersion`
for StoragePolicy (STORAGE_POLICY_VERSION), which can be used to reject
requests when the user passes an explicit storage policy? We can set a default
post this check if OzoneManagerVersion == STORAGE_POLICY_VERSION in `RpcClient`?
2. Should the same approach be followed for all client side code related to
StoragePolicy? (Example: `PutKeyHandler` etc..)
3. If we do create a new version, how do we handle `UpdateBucketHandler` ?
Null storage policy here represents unsetting the storagePolicy of a bucket.
@devmadhuu what do you think?
--
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]
