devmadhuu commented on code in PR #11232:
URL: https://github.com/apache/ozone/pull/11232#discussion_r4101818541
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java:
##########
@@ -377,6 +378,17 @@ public void setStoragePolicy(StoragePolicy
newStoragePolicy) throws IOException
storagePolicy = newStoragePolicy;
}
+ /**
+ * Sets the bucket's storage-policy properties carried in the given
+ * {@link OmBucketArgs} (storage policy, allowFallback, or unset). Used by
the
+ * update path, where the policy may be absent or explicitly cleared.
+ * @param args Bucket arguments carrying the properties to update.
+ * @throws IOException
+ */
+ public void setStoragePolicyProperty(OmBucketArgs args) throws IOException {
Review Comment:
This is a public method and any client can use with arguments for different
volume/bucket than its own bucket object.
Assume we load bucket A:
```
OzoneBucket bucketA =
objectStore.getVolume("vol1").getBucket("photos");
```
Internally:
```
bucketA.volumeName = "vol1"
bucketA.name = "photos"
```
Now someone constructs arguments for a completely different bucket:
```
OmBucketArgs argsForBucketB =
OmBucketArgs.newBuilder()
.setVolumeName("vol2")
.setBucketName("backups")
.setStoragePolicy(OzoneStoragePolicy.COLD)
.build();
```
Then calls:
`bucketA.setStoragePolicyProperty(argsForBucketB);`
Logically, because the method is being invoked on bucketA, a caller would
expect it to modify bucket A:
vol1/photos
But the new method ignores bucket A’s identity:
```
public void setStoragePolicyProperty(OmBucketArgs args) {
proxy.setBucketStoragePolicy(args);
}
```
The supplied args contains:
```
volumeName = vol2
bucketName = backups
```
RpcClient also trusts those values:
```
public void setBucketStoragePolicy(OmBucketArgs args)
throws IOException {
Objects.requireNonNull(args, "args == null");
verifyVolumeName(args.getVolumeName());
verifyBucketName(args.getBucketName());
ozoneManagerClient.setBucketProperty(args);
}
```
And also getters on existing bucket object can return stale values. No one
calls below getters to update `this` object values.
```
storagePolicy = args.getStoragePolicy();
allowFallbackStoragePolicy =
args.getAllowFallbackStoragePolicy();
Basically , a direct Java API caller would see the incorrect old value.
```
so I would recommend using like below:
```
public void setStoragePolicyProperty(
StoragePolicy newPolicy,
Boolean newFallback,
boolean unsetPolicy) throws IOException {
OmBucketArgs.Builder builder = OmBucketArgs.newBuilder()
.setVolumeName(volumeName)
.setBucketName(name);
if (newPolicy != null) {
builder.setStoragePolicy(newPolicy);
}
if (newFallback != null) {
builder.setAllowFallbackStoragePolicy(newFallback);
}
if (unsetPolicy) {
builder.setUnsetStoragePolicy(true);
}
proxy.setBucketStoragePolicy(builder.build());
if (unsetPolicy) {
storagePolicy = null;
} else if (newPolicy != null) {
storagePolicy = newPolicy;
}
if (newFallback != null) {
allowFallbackStoragePolicy = newFallback;
}
}
```
##########
hadoop-ozone/cli-shell/src/main/java/org/apache/hadoop/ozone/shell/bucket/UpdateBucketHandler.java:
##########
@@ -35,6 +39,21 @@ public class UpdateBucketHandler extends BucketHandler {
description = "Owner of the bucket to set")
private String ownerName;
+ @Option(names = {"--storage-policy", "-s"},
+ description = "Bucket StoragePolicy. Allowed values: HOT, WARM, COLD,
null "
+ + "(null clears the bucket's StoragePolicy). Leave unset to keep the
"
+ + "current value.")
+ private String storagePolicyStr;
+
+ @Option(names = {"--allow-fallback-storage-policy", "-a"},
+ description = "When true, allocation may fall back to the
StoragePolicy's "
+ + "fallback tier if the creation tier is unavailable. Leave unset to
"
+ + "keep the current value.",
+ arity = "1")
+ private Boolean allowFallBackStoragePolicy;
Review Comment:
Before this PR, there was only one property "ownerName" was allowed to
update on a bucket, but now with this PR, it is allowing to update multiple
properties and that too sequentially in multiple RPC requests but there is no
guarantee that update to both properties for a bucket will be applied
successfully. If one fails after other applied successfully, this can update
metadata for a bucket in inconsistent state in OM which is what may not be
desired by user/client updating multiple properties on a bucket. See this
[code](https://github.com/apache/ozone/blob/3d6aa1bd0fcca842ac6d2fccbb48d9cb167d06b6/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java#L172).
So ideally we should find a way to atomically update multiple properties if
client is trying to set on a bucket.
That means a request containing:
```
ownerName = bob
storagePolicy = COLD
```
is routed to:
`OMBucketSetOwnerRequest`
and not the general:
`OMBucketSetPropertyRequest`
So even if we put both fields into one request, that is not enough.
Supporting an atomic combined update would require changing the OM request
routing and server-side handling which is beyond the scope of this PR. So my
recommendation would be for now in this PR .
**### Option 1: validate first**
This is the smallest fix:
```
Parse storage policy
|
+-- invalid --> fail with no mutation
|
+-- valid --> perform owner and policy RPCs
```
It fixes the reported INVALID scenario, but the two RPCs remain non-atomic.
**### Option 2: reject combining owner and storage-policy options**
This is the safest surgical behavior:
```
boolean hasStorageUpdate =
hasStoragePolicy()
|| allowFallBackStoragePolicy != null;
if (ownerName != null && hasStorageUpdate) {
throw new IllegalArgumentException(
"--user cannot be combined with storage-policy options");
}
```
Then users run separate commands:
```
ozone sh bucket update vol1/photos --user bob
ozone sh bucket update vol1/photos --storage-policy COLD
```
Each command performs one logical remote mutation.
Also then add some tests where command will have two individual properties
being tried to set and fail intentionally one by passing invalid args for
storage policy and then assert the expected value after update.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -901,6 +901,7 @@ message BucketArgs {
repeated hadoop.hdds.KeyValue tags = 13;
optional hadoop.hdds.StoragePolicyProto storagePolicy = 14;
optional bool allowFallbackStoragePolicy = 15;
+ optional bool unsetStoragePolicy = 16;
Review Comment:
This field is protobuf-compatible, but older OMs do not understand its
meaning. During a rolling upgrade, a new OM would clear the policy when
`unsetStoragePolicy=true`, while an old OM would ignore field 16 and retain the
policy. This can make OM HA members apply the same Ratis request differently.
Could we add an OM storage-policy layout feature and reject create/update
requests containing `storagePolicy`, `allowFallbackStoragePolicy`, or
`unsetStoragePolicy` while that feature is not finalized? The existing EC
validator in `OMBucketSetPropertyRequest` demonstrates the expected
`CLUSTER_NEEDS_FINALIZATION` pattern. Please also add an upgrade test covering
rejection before finalization and successful
update after finalization.
--
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]
