[
https://issues.apache.org/jira/browse/HDDS-16947?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrey Yarovoy updated HDDS-16947:
----------------------------------
Description:
`BasicRootedOzoneClientAdapterImpl.setTimes` calls `getBucket()` (`InfoBucket`,
plus `InfoVolume` +
`InfoBucket` per link hop) before `OzoneBucket.setTimes`. So every
`FileSystem.setTimes` sends at least 2 OM
requests. The fetch does one job: `OzoneFSUtils.validateBucketLayout`, which
rejects OBJECT_STORE. Nothing
else is read from the bucket. `setTimes` is called once per file by tools that
preserve times, for
example `distcp -pt`, and through HttpFS.
OM does not check the layout today. `OMKeySetTimesRequest` is registered for
`BucketLayout.OBJECT_STORE`
in `BucketLayoutAwareOMKeyRequestFactory` (that registration serves LEGACY as
well), so `SetTimes`
succeeds on an OBJECT_STORE bucket.
*Why the rejection is opt-in*
No in-tree object store caller sends `SetTimes`: nothing in `s3gateway`,
`cli-shell`, `freon` or `recon`.
The only callers are the o3fs and ofs file systems, and HttpFS through them.
But `OzoneBucket.setTimes` is
public Java API, and it works on OBJECT_STORE buckets today. An unconditional
rejection would break that.
The client asks for the check:
{code:java}
message SetTimesRequest {
required KeyArgs keyArgs = 1;
required uint64 mtime = 2;
required uint64 atime = 3;
// Set by file system clients: OM rejects the request with
// NOT_SUPPORTED_OPERATION if the resolved bucket layout is OBJECT_STORE.
optional bool requireFileSystemLayout = 4;
}
{code}
*Proposed change*
*OM.* Check before Ratis, on the leader. Do it in
`OMKeySetTimesRequest.preExecute`, or right after
`createClientRequest` in `OMExecutionFlow.submitExecutionToRatis`. Use the
link-resolved layout that
`BucketLayoutAwareOMKeyRequestFactory.createRequest` has just read through
`OzoneManagerUtils.getBucketLayout`, so no new lookup is added. Reuse
`OzoneFSUtils.validateBucketLayout` for the message and wrap it in
`OMException(NOT_SUPPORTED_OPERATION)`, as
`OmMetadataReader.validateFileSystemLayout` does.
Do not put the check inside `createRequest` itself, because that also runs at
apply time on every OM
(`OzoneManagerRequestHandler.handleWriteRequestImpl`). A check there would be
deterministic: the client
only sets the field once the OM reports the new version, which happens after
finalization. But a
rejection at apply still costs a Ratis log entry, and a pre-Ratis rejection
costs nothing. Note:
`OMKeySetTimesRequest.preExecute` rebuilds `KeyArgs` with
`KeyArgs.newBuilder()`, so it does not carry
the field forward. That is fine if the check runs before the rebuild.
*Client.*
* `OmKeyArgs` already has `requireFileSystemLayout` (HDDS-16760).
`OzoneManagerProtocolClientSideTranslatorPB.setTimes`
builds `KeyArgs` field by field, so add the `SetTimesRequest` field there.
* `ClientProtocol` / `OzoneBucket` need a way for the adapter to set it.
Mirror what HDDS-16760 did with
`ListStatusLightOptions`, or add an overload that takes `OmKeyArgs`.
* The adapter skips `getBucket()` when `proxy.getOmVersion() >=` a new
`OzoneManagerVersion` constant.
Place the constant after `LIST_STATUS_REJECTS_OBS(101)`; the number is assigned
at merge, so
coordinate it with sibling Jiras. Never put it below `ZDU(100)`, because an OM
without this change can
report ZDU once finalized. It then ignores the field.
* Map `NOT_SUPPORTED_OPERATION` to `IllegalArgumentException`.
* Against any older OM, including one that reports ZDU, keep the current
`getBucket()` path.
*Compatibility*
| |Behavior|
|—|—|
|New client, new OM|1 RPC; OM rejects OBJECT_STORE|
|New client, older OM (incl. ZDU)|`getBucket()` + `SetTimes`, as today|
|Old client / direct `OzoneBucket.setTimes`, new OM|field absent ⇒ unchanged,
OBJECT_STORE still accepted|
There is one new optional proto field, no new result code and no new
configuration. `proto.lock` is not
updated (HDDS-14779 / HDDS-16760 precedent).
*Testing*
* Adapter (Mockito):
* exactly one OM call, carrying the field;
*`NOT_SUPPORTED_OPERATION` → `IllegalArgumentException`;
* an older OM, including `ZDU`, falls back to `getBucket()`.
* OM request test, for each layout:
* with the field set, OBJECT_STORE is rejected before Ratis, while FSO and
*{*}LEGACY{*}* still succeed.
LEGACY guards against the `getKey(Type, BucketLayout)` suffix subtlety:
OBJECT_STORE-registered
classes also serve LEGACY, so test the layout value, never a missing map entry;
* without the field, OBJECT_STORE still succeeds;
* a link to an OBJECT_STORE bucket is rejected, which proves the resolved
layout is used.
* Translator/handler: the field round-trips.
* `AbstractRootedOzoneFileSystemTest`: `setTimes` on an OBJECT_STORE bucket
fails with
`IllegalArgumentException`. Keep RPC counting in the adapter test, not on
OMMetrics deltas (HDDS-16513).
* Benchmark: `TestOfsSetTimesRpcBenchmark`, same shape as
`TestOfsListStatusRpcBenchmark`:
* 200 FSO buckets, shuffled accesses, single-threaded and 10 threads;
* baseline vs change, n=3 per arm, built and run strictly one after the other;
* invariant on the new arm: exactly 1 OM RPC per `setTimes`, from 2 before.
was:
`BasicRootedOzoneClientAdapterImpl.setTimes` calls `getBucket()` (`InfoBucket`,
plus `InfoVolume` +
`InfoBucket` per link hop) before `OzoneBucket.setTimes`. So every
`FileSystem.setTimes` sends at least 2 OM
requests. The fetch does one job: `OzoneFSUtils.validateBucketLayout`, which
rejects OBJECT_STORE. Nothing
else is read from the bucket. `setTimes` is called once per file by tools that
preserve times, for
example `distcp -pt`, and through HttpFS.
OM does not check the layout today. `OMKeySetTimesRequest` is registered for
`BucketLayout.OBJECT_STORE`
in `BucketLayoutAwareOMKeyRequestFactory` (that registration serves LEGACY as
well), so `SetTimes`
succeeds on an OBJECT_STORE bucket.
### Why the rejection is opt-in
No in-tree object store caller sends `SetTimes`: nothing in `s3gateway`,
`cli-shell`, `freon` or `recon`.
The only callers are the o3fs and ofs file systems, and HttpFS through them.
But `OzoneBucket.setTimes` is
public Java API, and it works on OBJECT_STORE buckets today. An unconditional
rejection would break that.
The client asks for the check:
{code}
message SetTimesRequest {
required KeyArgs keyArgs = 1;
required uint64 mtime = 2;
required uint64 atime = 3;
// Set by file system clients: OM rejects the request with
// NOT_SUPPORTED_OPERATION if the resolved bucket layout is OBJECT_STORE.
optional bool requireFileSystemLayout = 4;
}
{code}
### Proposed change
*OM.* Check before Ratis, on the leader. Do it in
`OMKeySetTimesRequest.preExecute`, or right after
`createClientRequest` in `OMExecutionFlow.submitExecutionToRatis`. Use the
link-resolved layout that
`BucketLayoutAwareOMKeyRequestFactory.createRequest` has just read through
`OzoneManagerUtils.getBucketLayout`, so no new lookup is added. Reuse
`OzoneFSUtils.validateBucketLayout` for the message and wrap it in
`OMException(NOT_SUPPORTED_OPERATION)`, as
`OmMetadataReader.validateFileSystemLayout` does.
Do not put the check inside `createRequest` itself, because that also runs at
apply time on every OM
(`OzoneManagerRequestHandler.handleWriteRequestImpl`). A check there would be
deterministic: the client
only sets the field once the OM reports the new version, which happens after
finalization. But a
rejection at apply still costs a Ratis log entry, and a pre-Ratis rejection
costs nothing. Note:
`OMKeySetTimesRequest.preExecute` rebuilds `KeyArgs` with
`KeyArgs.newBuilder()`, so it does not carry
the field forward. That is fine if the check runs before the rebuild.
*Client.*
* `OmKeyArgs` already has `requireFileSystemLayout` (HDDS-16760).
`OzoneManagerProtocolClientSideTranslatorPB.setTimes`
builds `KeyArgs` field by field, so add the `SetTimesRequest` field there.
* `ClientProtocol` / `OzoneBucket` need a way for the adapter to set it. Mirror
what HDDS-16760 did with
`ListStatusLightOptions`, or add an overload that takes `OmKeyArgs`.
* The adapter skips `getBucket()` when `proxy.getOmVersion() >=` a new
`OzoneManagerVersion` constant.
Place the constant after `LIST_STATUS_REJECTS_OBS(101)`; the number is assigned
at merge, so
coordinate it with sibling Jiras. Never put it below `ZDU(100)`, because an OM
without this change can
report ZDU once finalized. It then ignores the field.
* Map `NOT_SUPPORTED_OPERATION` to `IllegalArgumentException`.
* Against any older OM, including one that reports ZDU, keep the current
`getBucket()` path.
### Compatibility
| | Behavior |
|---|---|
| New client, new OM | 1 RPC; OM rejects OBJECT_STORE |
| New client, older OM (incl. ZDU) | `getBucket()` + `SetTimes`, as today |
| Old client / direct `OzoneBucket.setTimes`, new OM | field absent ⇒
unchanged, OBJECT_STORE still accepted |
There is one new optional proto field, no new result code and no new
configuration. `proto.lock` is not
updated (HDDS-14779 / HDDS-16760 precedent).
Points for review: the BUCKET/READ ACL check from `getBucketDetails` is
dropped. `OMKeySetTimesRequest`
keeps its own key ACL check, and OM checks READ on each link it follows.
## Testing
* Adapter (Mockito):
* exactly one OM call, carrying the field;
*`NOT_SUPPORTED_OPERATION` → `IllegalArgumentException`;
* an older OM, including `ZDU`, falls back to `getBucket()`.
* OM request test, for each layout:
* with the field set, OBJECT_STORE is rejected before Ratis, while FSO and
**LEGACY** still succeed.
LEGACY guards against the `getKey(Type, BucketLayout)` suffix subtlety:
OBJECT_STORE-registered
classes also serve LEGACY, so test the layout value, never a missing map entry;
* without the field, OBJECT_STORE still succeeds;
* a link to an OBJECT_STORE bucket is rejected, which proves the resolved
layout is used.
* Translator/handler: the field round-trips.
* `AbstractRootedOzoneFileSystemTest`: `setTimes` on an OBJECT_STORE bucket
fails with
`IllegalArgumentException`. Keep RPC counting in the adapter test, not on
OMMetrics deltas (HDDS-16513).
* Benchmark: `TestOfsSetTimesRpcBenchmark`, same shape as
`TestOfsListStatusRpcBenchmark`:
* 200 FSO buckets, shuffled accesses, single-threaded and 10 threads;
* baseline vs change, n=3 per arm, built and run strictly one after the other;
* invariant on the new arm: exactly 1 OM RPC per `setTimes`, from 2 before.
> Remove the per-call InfoBucket from rooted-ofs setTimes
> -------------------------------------------------------
>
> Key: HDDS-16947
> URL: https://issues.apache.org/jira/browse/HDDS-16947
> Project: Apache Ozone
> Issue Type: Improvement
> Components: Ozone Client
> Reporter: Andrey Yarovoy
> Assignee: Andrey Yarovoy
> Priority: Major
>
> `BasicRootedOzoneClientAdapterImpl.setTimes` calls `getBucket()`
> (`InfoBucket`, plus `InfoVolume` +
> `InfoBucket` per link hop) before `OzoneBucket.setTimes`. So every
> `FileSystem.setTimes` sends at least 2 OM
> requests. The fetch does one job: `OzoneFSUtils.validateBucketLayout`, which
> rejects OBJECT_STORE. Nothing
> else is read from the bucket. `setTimes` is called once per file by tools
> that preserve times, for
> example `distcp -pt`, and through HttpFS.
> OM does not check the layout today. `OMKeySetTimesRequest` is registered for
> `BucketLayout.OBJECT_STORE`
> in `BucketLayoutAwareOMKeyRequestFactory` (that registration serves LEGACY as
> well), so `SetTimes`
> succeeds on an OBJECT_STORE bucket.
> *Why the rejection is opt-in*
> No in-tree object store caller sends `SetTimes`: nothing in `s3gateway`,
> `cli-shell`, `freon` or `recon`.
> The only callers are the o3fs and ofs file systems, and HttpFS through them.
> But `OzoneBucket.setTimes` is
> public Java API, and it works on OBJECT_STORE buckets today. An unconditional
> rejection would break that.
>
> The client asks for the check:
> {code:java}
> message SetTimesRequest {
> required KeyArgs keyArgs = 1;
> required uint64 mtime = 2;
> required uint64 atime = 3;
> // Set by file system clients: OM rejects the request with
> // NOT_SUPPORTED_OPERATION if the resolved bucket layout is OBJECT_STORE.
> optional bool requireFileSystemLayout = 4;
> }
> {code}
> *Proposed change*
> *OM.* Check before Ratis, on the leader. Do it in
> `OMKeySetTimesRequest.preExecute`, or right after
> `createClientRequest` in `OMExecutionFlow.submitExecutionToRatis`. Use the
> link-resolved layout that
> `BucketLayoutAwareOMKeyRequestFactory.createRequest` has just read through
> `OzoneManagerUtils.getBucketLayout`, so no new lookup is added. Reuse
> `OzoneFSUtils.validateBucketLayout` for the message and wrap it in
> `OMException(NOT_SUPPORTED_OPERATION)`, as
> `OmMetadataReader.validateFileSystemLayout` does.
> Do not put the check inside `createRequest` itself, because that also runs at
> apply time on every OM
> (`OzoneManagerRequestHandler.handleWriteRequestImpl`). A check there would be
> deterministic: the client
> only sets the field once the OM reports the new version, which happens after
> finalization. But a
> rejection at apply still costs a Ratis log entry, and a pre-Ratis rejection
> costs nothing. Note:
> `OMKeySetTimesRequest.preExecute` rebuilds `KeyArgs` with
> `KeyArgs.newBuilder()`, so it does not carry
> the field forward. That is fine if the check runs before the rebuild.
> *Client.*
> * `OmKeyArgs` already has `requireFileSystemLayout` (HDDS-16760).
> `OzoneManagerProtocolClientSideTranslatorPB.setTimes`
> builds `KeyArgs` field by field, so add the `SetTimesRequest` field there.
> * `ClientProtocol` / `OzoneBucket` need a way for the adapter to set it.
> Mirror what HDDS-16760 did with
> `ListStatusLightOptions`, or add an overload that takes `OmKeyArgs`.
> * The adapter skips `getBucket()` when `proxy.getOmVersion() >=` a new
> `OzoneManagerVersion` constant.
> Place the constant after `LIST_STATUS_REJECTS_OBS(101)`; the number is
> assigned at merge, so
> coordinate it with sibling Jiras. Never put it below `ZDU(100)`, because an
> OM without this change can
> report ZDU once finalized. It then ignores the field.
> * Map `NOT_SUPPORTED_OPERATION` to `IllegalArgumentException`.
> * Against any older OM, including one that reports ZDU, keep the current
> `getBucket()` path.
> *Compatibility*
> | |Behavior|
> |—|—|
> |New client, new OM|1 RPC; OM rejects OBJECT_STORE|
> |New client, older OM (incl. ZDU)|`getBucket()` + `SetTimes`, as today|
> |Old client / direct `OzoneBucket.setTimes`, new OM|field absent ⇒ unchanged,
> OBJECT_STORE still accepted|
> There is one new optional proto field, no new result code and no new
> configuration. `proto.lock` is not
> updated (HDDS-14779 / HDDS-16760 precedent).
> *Testing*
> * Adapter (Mockito):
> * exactly one OM call, carrying the field;
> *`NOT_SUPPORTED_OPERATION` → `IllegalArgumentException`;
> * an older OM, including `ZDU`, falls back to `getBucket()`.
> * OM request test, for each layout:
> * with the field set, OBJECT_STORE is rejected before Ratis, while FSO and
> *{*}LEGACY{*}* still succeed.
> LEGACY guards against the `getKey(Type, BucketLayout)` suffix subtlety:
> OBJECT_STORE-registered
> classes also serve LEGACY, so test the layout value, never a missing map
> entry;
> * without the field, OBJECT_STORE still succeeds;
> * a link to an OBJECT_STORE bucket is rejected, which proves the resolved
> layout is used.
> * Translator/handler: the field round-trips.
> * `AbstractRootedOzoneFileSystemTest`: `setTimes` on an OBJECT_STORE bucket
> fails with
> `IllegalArgumentException`. Keep RPC counting in the adapter test, not on
> OMMetrics deltas (HDDS-16513).
> * Benchmark: `TestOfsSetTimesRpcBenchmark`, same shape as
> `TestOfsListStatusRpcBenchmark`:
> * 200 FSO buckets, shuffled accesses, single-threaded and 10 threads;
> * baseline vs change, n=3 per arm, built and run strictly one after the
> other;
> * invariant on the new arm: exactly 1 OM RPC per `setTimes`, from 2 before.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
