devmadhuu commented on code in PR #11626:
URL: https://github.com/apache/ozone/pull/11626#discussion_r4376339354
##########
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?
@anuragp010 , I agree that an explicitly requested storage policy should not
be silently ignored. A few points:
1. MPU currently preserves the distinction between explicit and unspecified
policy.
`RpcClient.initiateMultipartUpload()` passes null when the user supplies
no policy, and the translator omits the field. Client-side default insertion
exists for bucket creation, but not this MPU path.
I suggest preserving this behavior:
Explicit policy → use it
No policy → inherit bucket policy → otherwise use cluster default
For example, if the bucket is HOT and the user supplies no policy,
automatically sending WARM would incorrectly override the bucket’s HOT policy.
Check last point #4 for more clarity.
2. An OzoneManagerVersion entry would help with older OMs, but we still need
server-side finalization checks.
These are separate checks:
- Client-side version check: does the OM software understand storage
policies?
- Server-side layout check: has the cluster finalized storage-policy
support?
An OM can run the new software while the upgrade remains unfinalized.
Therefore, I suggest:
- Reject explicit policies against unsupported older OMs.
- Reject explicit policies before finalization with
NOT_SUPPORTED_OPERATION_PRIOR_FINALIZATION.
- Allow requests without a policy to retain legacy behavior before
finalization.
- Use a minimum-version comparison (>=), not equality, so future versions
remain supported.
3. Yes, apply this consistently across storage-policy APIs.
Shared `RpcClient` methods should perform the compatibility checks, rather
than only CLI handlers such as `PutKeyHandler`, so Java callers receive the
same behavior. However, defaulting should remain operation-specific—not applied
blindly everywhere.
4. For `UpdateBucketHandler`, the existing unset flag already removes the
ambiguity.
- No policy and no unset flag: leave the policy unchanged.
- A supplied policy: set that policy.
- unsetStoragePolicy=true: remove the policy.
The CLI’s "null" option becomes an explicit unset request. We should
preserve that flag, check whether the operation is supported, and never replace
it with WARM.
In short, I support adding the version check, while preserving absent-policy
semantics and retaining server-side finalization enforcement.
--
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]
