You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "yandrey321 (via GitHub)" <gi...@apache.org> on 2026/10/01 22:21:19 UTC
[PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
yandrey321 opened a new pull request, #11380:
URL: https://github.com/apache/ozone/pull/11380
## What changes were proposed in this pull request?
`BasicRootedOzoneClientAdapterImpl.getFileChecksum` issues three OM RPCs per call:
`InfoVolume`, `InfoBucket`, and `LookupKey`. Through a link bucket it is five, because the link
resolution repeats the volume and bucket lookups. Only the key lookup carries information the
checksum needs — the volume and bucket objects were used solely for `getName()`.
The `InfoBucket` call existed only to reject OBJECT_STORE buckets, which have no file system
semantics. This patch moves that check to the server, where the bucket is already resolved, and the
other two RPCs then have no reason to exist.
### Server side
`OmMetadataReader.lookupFile` now validates the resolved bucket layout and rejects OBJECT_STORE,
mirroring what HDDS-15925 (#11226) did for `getFileStatus`. `OzoneFSUtils.validateBucketLayout`'s
`IllegalArgumentException` is wrapped as `OMException(NOT_SUPPORTED_OPERATION)` so it reaches the
client as a normal, non-retryable RPC response instead of escaping the read handler's `IOException`
catch and triggering a client retry storm.
`lookupFile` is the right home for the check: it is the file system twin of `lookupKey`, it already
resolves the bucket, and it previously performed no layout validation at all. `lookupKey` cannot take
the check, because it is shared with OBS and S3 reads.
`lookupFile` is otherwise equivalent to `lookupKey` for these arguments — both honor
`getLatestVersionLocation()`, call `refresh()` and `sortDatanodes()`, check `ResourceType.KEY`/`READ`,
and apply `normalizeKeyArgs(bucket.update(args), bucket)`. Both also call `addBlockToken4Read`, which
is why the checksum cannot instead be routed through `getFileStatus`:
`KeyManagerImpl.getOzoneFileStatus{,FSO}` never adds block tokens, so the returned locations would be
unusable in secure mode.
### Client side
`OzoneClientUtils.getFileChecksumWithCombineMode` now takes `(volumeName, bucketName, keyName, …)`
plus a `useLookupFile` flag instead of `OzoneVolume`/`OzoneBucket`. The name-based signature is pushed
down through `ChecksumHelperFactory`, `BaseFileChecksumHelper` and both subclasses, which removes the
implicit coupling that the helpers may only ever call `getName()` on those objects.
The ofs adapter gates on the negotiated OM version:
```java
boolean omRejectsObs = proxy.getOmVersion()
.compareTo(OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS) >= 0;
```
New OM: one `LookupFile`. Pre-upgrade OM (a new client against an older server during a rolling
upgrade): keep the client-side layout check and use `LookupKey` — still two RPCs rather than three,
because the `InfoVolume` call is dropped unconditionally. `OzoneManagerUtils.reportNotFound` probes
the volume table and raises `VOLUME_NOT_FOUND` before falling back to `BUCKET_NOT_FOUND`, so dropping
it loses no error fidelity. The fallback retires itself as clusters upgrade.
`NOT_SUPPORTED_OPERATION` is mapped back to `IllegalArgumentException`, so the user-visible behavior
for an OBS bucket is unchanged — the same mapping #11226 uses for `getFileStatus`. `NOT_A_FILE`,
which is how `lookupFile` reports a directory where `lookupKey` reported `KEY_NOT_FOUND`, becomes
`FileNotFoundException`, matching HDFS.
`OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS(15, …)` is additive. There is no protobuf change.
o3fs (`BasicOzoneClientAdapterImpl`) and `ozone sh key checksum` deliberately stay on `LookupKey`:
o3fs validates the layout once in its constructor and has no `InfoBucket` to save on this path, and
the CLI must keep working against OBS buckets.
### Relation to [PR 11369](https://github.com/apache/ozone/pull/11369)
[PR 11369](https://github.com/apache/ozone/pull/11369) addresses the same three RPCs from the client side: it drops the `InfoVolume` call and adds a
per-bucket layout cache (`ozone.client.fs.bucket.layout.cache.expiry`, default 2m;
`ozone.client.fs.bucket.layout.cache.size`, default 1000) so the `InfoBucket` call is paid once per
bucket instead of once per call.
The two are not in conflict, and #11369's first half — dropping `InfoVolume`, which is pure
redundancy — is carried here. They differ in what the remaining `InfoBucket` cost becomes:
| | master | #11369 (client cache) | this PR (server side) |
|---|---|---|---|
| OM RPCs per call | 3.00 | 2 − hitRatio | **1.00** |
| …at a 90% hit ratio | 3.00 | 1.10 | **1.00** |
| …cold client, or more buckets than the cache holds | 3.00 | 2.00 | **1.00** |
| Staleness window | none | 2 min (default) | none |
| New configuration | none | 2 keys | none |
| Client memory | none | up to 1000 layouts | none |
| Works against an un-upgraded OM | — | yes | falls back to 2.00 |
At the hit ratio #11369 is designed for, the RPC counts are close (1.10 vs 1.00) and so is the
measured latency. The case for doing this server-side is that 1.00 is unconditional: there is no
hit-ratio cliff for a cold client or a bucket count above the cache size, no window in which a
deleted-and-recreated bucket resolves against a stale layout, and nothing to configure or invalidate.
The cost is that the single-RPC path needs the OM upgraded; until then the client falls back to two
RPCs, which is still better than master.
A secondary benefit of putting the check in `lookupFile` is that it is not client-specific: any
caller that reaches `lookupFile` on an OBS bucket now gets a clear `NOT_SUPPORTED_OPERATION` instead
of file system semantics silently applied to a bucket that has none.
Workload adapted from #11369's own benchmark so all three arms run identical test code:
2000 `getFileChecksum` calls over 200 FSO buckets, 10 accesses per bucket from a seed-shuffled
sequence (a 90% cache-hit workload — #11369's best case), 4 KiB files, 3 datanodes,
`MiniOzoneCluster` on loopback. Each arm was built and installed separately and run 3 times;
figures are medians of 3.
### OM RPCs per call — exact, identical in all 9 runs
| | master | #11369 | this PR |
|---|---|---|---|
| InfoVolume | 2000 | 0 | **0** |
| InfoBucket | 2000 | 200 | **0** |
| LookupKey | 2000 | 2000 | 0 |
| LookupFile | 0 | 0 | 2000 |
| **per call** | **3.00** | **1.10** | **1.00** |
This is deterministic rather than a timing measurement. It also confirms the version gate negotiates
correctly, since the single-RPC path is taken only when the OM advertises `LOOKUP_FILE_REJECTS_OBS`.
### Latency and throughput (medians of 3)
Single-threaded:
| | master | #11369 | this PR | vs master | vs #11369 |
|---|---|---|---|---|---|
| mean | 0.704 ms | 0.556 ms | 0.525 ms | **−25.4%** | −5.6% |
| p50 | 0.674 ms | 0.508 ms | 0.506 ms | −24.9% | −0.4% (tie) |
| p99 | 1.008 ms | 0.953 ms | 0.799 ms | −20.7% | −16.2% |
| throughput | 1420 ops/s | 1798 ops/s | 1903 ops/s | **+34.0%** | +5.9% |
10 concurrent threads sharing one `FileSystem`:
| | master | #11369 | this PR | vs master | vs #11369 |
|---|---|---|---|---|---|
| mean | 2.324 ms | 1.785 ms | 1.730 ms | **−25.6%** | −3.1% |
| p50 | 2.240 ms | 1.701 ms | 1.648 ms | −26.4% | −3.1% |
| throughput | 4275 ops/s | 5517 ops/s | 5733 ops/s | **+34.1%** | +3.9% |
Every figure above except the single-threaded p50 has non-overlapping run ranges between the two
optimized arms, so the margins are small but reproducible. Run-to-run spread within an arm is under
4%.
### What the numbers do and do not support
- **Against master the win is unambiguous**: 3.00 → 1.00 RPCs per call, ~25% lower mean and p50
latency, ~34% higher throughput, reproduced in both concurrency regimes, the two regimes agreeing
within 0.3 pp on the mean delta.
- **Against #11369 on its best-case workload the margin is small** (3–6% on latency and throughput,
16% on single-threaded p99) and should not be the reason to prefer this approach. The reason is the
unconditional 1.00 and the absence of a staleness window.
- **Concurrent p99 is not resolvable at n=3** on any arm — the ranges overlap heavily (master
2.911–5.797 ms, #11369 3.179–6.378 ms, this PR 2.697–5.489 ms). Tail latency on a 10-thread
2000-call MiniCluster run is dominated by a handful of outliers, and it is not what this change
targets. No p99 claim is made for the concurrent case.
- **The cold-client row (2.00 for #11369) is derived, not measured** — it follows directly from
`2 − hitRatio` with no cache hits. The measured arm is the 90%-hit case only.
- Latency improves less than RPC count because each call still makes one `GetBlockChecksum` round
trip to a datanode. Two of roughly four hops are removed, and the two removed are cheap
full-cache in-memory volume and bucket reads, which lands at about a quarter.
Generated-by: Claude Code (claude-opus)
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15951
## How was this patch tested?
New unit tests in `TestOMMetadataReader` — `lookupFileRejectsObjectStoreLayout`,
`lookupFileAllowsLegacyLayout` — mirroring the #11226 pair.
Three tests added to `AbstractRootedOzoneFileSystemTest`, so they run under `TestOFS`,
`TestOFSWithFSPaths` and `TestOFSWithFSO` on the existing shared cluster rather than standing up
another one:
- `testGetFileChecksumUsesSingleOmRpc` — asserts `numBucketInfos`, `numVolumeInfos` and
`numKeyLookups` are all unchanged across a `getFileChecksum`. The last of these is what proves
`LookupFile` replaced `LookupKey`.
- `testGetFileChecksumRejectsObsBucket`
- `testGetFileChecksumOnDirectory`
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #11380:
URL: https://github.com/apache/ozone/pull/11380#discussion_r4211804455
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java:
##########
@@ -268,8 +268,9 @@ public void testListStatusWithNotAllowedUser(BucketLayout bucketLayout,
recursive, keyName, numEntries, allowPartialPrefixes));
}
+ // LookupFile is rejected on OBJECT_STORE buckets, which have no file system semantics.
@ParameterizedTest
- @EnumSource(BucketLayout.class)
+ @EnumSource(value = BucketLayout.class, names = {"FILE_SYSTEM_OPTIMIZED", "LEGACY"})
Review Comment:
Should be fixed now
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java:
##########
@@ -281,8 +282,9 @@ public void testLookupFileWithAllowedUser(BucketLayout bucketLayout)
() -> ozoneManager.lookupFile(snapshotKeyArgs));
}
+ // LookupFile is rejected on OBJECT_STORE buckets, which have no file system semantics.
@ParameterizedTest
- @EnumSource(BucketLayout.class)
+ @EnumSource(value = BucketLayout.class, names = {"FILE_SYSTEM_OPTIMIZED", "LEGACY"})
Review Comment:
fixed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #11380:
URL: https://github.com/apache/ozone/pull/11380
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11380:
URL: https://github.com/apache/ozone/pull/11380#issuecomment-6064861336
Merged. Thanks @yandrey321 @lpavanvenkat
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11380:
URL: https://github.com/apache/ozone/pull/11380#issuecomment-5954980777
Fixed 2 integration tests that called lookup file for OBS
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on PR #11380:
URL: https://github.com/apache/ozone/pull/11380#issuecomment-6046562687
@lpavanvenkat could you please check updated code?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "aryangupta1998 (via GitHub)" <gi...@apache.org>.
aryangupta1998 commented on PR #11380:
URL: https://github.com/apache/ozone/pull/11380#issuecomment-5961057924
@lpavanvenkat can you please take a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #11380:
URL: https://github.com/apache/ozone/pull/11380#discussion_r4161427429
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java:
##########
@@ -672,10 +672,12 @@ public FileChecksum getFileChecksum(String keyName, long length)
if (combineMode == null) {
return null;
}
+ // The bucket layout is validated once in the constructor, so LookupKey is
+ // enough here; there is no InfoBucket RPC to save on this path.
return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, keyName,
+ volume.getName(), bucket.getName(), keyName,
length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
+ ozoneClient.getObjectStore().getClientProxy(), false);
Review Comment:
if the last parameter is false, it calls lookupKey() not lookupFile.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #11380:
URL: https://github.com/apache/ozone/pull/11380#discussion_r4211806295
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
+ .compareTo(OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS) >= 0;
+ if (!omRejectsObs) {
+ // Called for its layout validation side effect; the bucket itself is not needed.
+ getBucket(ofsPath, false);
+ }
+ try {
+ return OzoneClientUtils.getFileChecksumWithCombineMode(
+ ofsPath.getVolumeName(), ofsPath.getBucketName(), ofsPath.getKeyName(),
+ length, combineMode,
+ ozoneClient.getObjectStore().getClientProxy(), omRejectsObs);
+ } catch (OMException e) {
+ if (e.getResult() == OMException.ResultCodes.NOT_SUPPORTED_OPERATION) {
+ // OM rejects LookupFile on an OBJECT_STORE bucket (no file system
+ // semantics). Surface it as IllegalArgumentException, matching the
+ // pre-HDDS-15951 client-side layout check.
+ throw new IllegalArgumentException(e.getMessage());
+ } else if (e.getResult() == OMException.ResultCodes.NOT_A_FILE) {
Review Comment:
Fixed
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
+ .compareTo(OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS) >= 0;
+ if (!omRejectsObs) {
+ // Called for its layout validation side effect; the bucket itself is not needed.
+ getBucket(ofsPath, false);
+ }
+ try {
+ return OzoneClientUtils.getFileChecksumWithCombineMode(
+ ofsPath.getVolumeName(), ofsPath.getBucketName(), ofsPath.getKeyName(),
+ length, combineMode,
+ ozoneClient.getObjectStore().getClientProxy(), omRejectsObs);
+ } catch (OMException e) {
+ if (e.getResult() == OMException.ResultCodes.NOT_SUPPORTED_OPERATION) {
+ // OM rejects LookupFile on an OBJECT_STORE bucket (no file system
+ // semantics). Surface it as IllegalArgumentException, matching the
+ // pre-HDDS-15951 client-side layout check.
+ throw new IllegalArgumentException(e.getMessage());
+ } else if (e.getResult() == OMException.ResultCodes.NOT_A_FILE) {
+ // LookupFile reports a directory this way; a checksum only exists for a
+ // file, so report it the same way HDFS does.
+ throw new FileNotFoundException(e.getMessage());
Review Comment:
Fixed
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
Review Comment:
Fixed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "lpavanvenkat (via GitHub)" <gi...@apache.org>.
lpavanvenkat commented on code in PR #11380:
URL: https://github.com/apache/ozone/pull/11380#discussion_r4205394710
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java:
##########
@@ -268,8 +268,9 @@ public void testListStatusWithNotAllowedUser(BucketLayout bucketLayout,
recursive, keyName, numEntries, allowPartialPrefixes));
}
+ // LookupFile is rejected on OBJECT_STORE buckets, which have no file system semantics.
@ParameterizedTest
- @EnumSource(BucketLayout.class)
+ @EnumSource(value = BucketLayout.class, names = {"FILE_SYSTEM_OPTIMIZED", "LEGACY"})
Review Comment:
On master this test ran for all `BucketLayout` values and asserted `lookupFile` succeeds, so `lookupFile` on an OBJECT_STORE bucket was a supported, tested operation. Excluding OBS here hides the behavior change rather than covering it. Could we keep OBS in the parameter set and assert that it now fails with `NOT_SUPPORTED_OPERATION`?
Since external `lookupFile` callers on OBS are affected (e.g. Freon `OmMetadataGenerator`, and clients older than `OPTIMIZED_GET_KEY_INFO`, whose `readFile` uses `lookupFile`), a release note like the one requested on #11226 would also help.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
+ .compareTo(OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS) >= 0;
+ if (!omRejectsObs) {
+ // Called for its layout validation side effect; the bucket itself is not needed.
+ getBucket(ofsPath, false);
+ }
+ try {
+ return OzoneClientUtils.getFileChecksumWithCombineMode(
+ ofsPath.getVolumeName(), ofsPath.getBucketName(), ofsPath.getKeyName(),
+ length, combineMode,
+ ozoneClient.getObjectStore().getClientProxy(), omRejectsObs);
+ } catch (OMException e) {
+ if (e.getResult() == OMException.ResultCodes.NOT_SUPPORTED_OPERATION) {
+ // OM rejects LookupFile on an OBJECT_STORE bucket (no file system
+ // semantics). Surface it as IllegalArgumentException, matching the
+ // pre-HDDS-15951 client-side layout check.
+ throw new IllegalArgumentException(e.getMessage());
+ } else if (e.getResult() == OMException.ResultCodes.NOT_A_FILE) {
Review Comment:
Only `NOT_A_FILE` is mapped to `FileNotFoundException` here. For a missing key, `lookupFile` throws `OMException(FILE_NOT_FOUND)`, which escapes as a raw `OMException`. So a directory yields `FileNotFoundException` but a missing file doesn't, which differs from HDFS.
Could we also map `FILE_NOT_FOUND` and `BUCKET_NOT_FOUND` to `FileNotFoundException`, the same way `getFileStatusForKeyOrSnapshot` does (and `KEY_NOT_FOUND` for the `lookupKey` fallback path), and add a test for a non-existent file?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOzoneManagerSnapshotAcl.java:
##########
@@ -281,8 +282,9 @@ public void testLookupFileWithAllowedUser(BucketLayout bucketLayout)
() -> ozoneManager.lookupFile(snapshotKeyArgs));
}
+ // LookupFile is rejected on OBJECT_STORE buckets, which have no file system semantics.
@ParameterizedTest
- @EnumSource(BucketLayout.class)
+ @EnumSource(value = BucketLayout.class, names = {"FILE_SYSTEM_OPTIMIZED", "LEGACY"})
Review Comment:
Same as above for the not-allowed-user variant.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
+ .compareTo(OzoneManagerVersion.LOOKUP_FILE_REJECTS_OBS) >= 0;
+ if (!omRejectsObs) {
+ // Called for its layout validation side effect; the bucket itself is not needed.
+ getBucket(ofsPath, false);
+ }
+ try {
+ return OzoneClientUtils.getFileChecksumWithCombineMode(
+ ofsPath.getVolumeName(), ofsPath.getBucketName(), ofsPath.getKeyName(),
+ length, combineMode,
+ ozoneClient.getObjectStore().getClientProxy(), omRejectsObs);
+ } catch (OMException e) {
+ if (e.getResult() == OMException.ResultCodes.NOT_SUPPORTED_OPERATION) {
+ // OM rejects LookupFile on an OBJECT_STORE bucket (no file system
+ // semantics). Surface it as IllegalArgumentException, matching the
+ // pre-HDDS-15951 client-side layout check.
+ throw new IllegalArgumentException(e.getMessage());
+ } else if (e.getResult() == OMException.ResultCodes.NOT_A_FILE) {
+ // LookupFile reports a directory this way; a checksum only exists for a
+ // file, so report it the same way HDFS does.
+ throw new FileNotFoundException(e.getMessage());
Review Comment:
Question: the description says `lookupKey` reported a directory as `KEY_NOT_FOUND`, but for FSO buckets `KeyManagerImpl.getOmKeyInfoFSO` seems to return the directory's key info rather than throw. Did `getFileChecksum` on an FSO directory previously return a result? If so, the switch to `FileNotFoundException` is a behavior change worth mentioning (it matches HDFS, so it's probably the right one).
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -1377,13 +1377,40 @@ public FileChecksum getFileChecksum(String keyName, long length)
return null;
}
OFSPath ofsPath = new OFSPath(keyName, config);
- OzoneVolume volume = objectStore.getVolume(ofsPath.getVolumeName());
- OzoneBucket bucket = getBucket(ofsPath, false);
- return OzoneClientUtils.getFileChecksumWithCombineMode(
- volume, bucket, ofsPath.getKeyName(),
- length, combineMode,
- ozoneClient.getObjectStore().getClientProxy());
-
+ if (ofsPath.getBucketName().isEmpty()) {
+ // throw FileNotFoundException in this case to make Hadoop common happy
+ throw new FileNotFoundException(
+ "getFileChecksum: Invalid argument: given bucket string is empty.");
+ }
+ // OM LookupFile rejects OBJECT_STORE buckets (HDDS-15951), so the volume and
+ // bucket no longer have to be fetched up front. During a rolling upgrade a new
+ // client can talk to an older OM that lacks that check, so when the negotiated
+ // OM version predates LOOKUP_FILE_REJECTS_OBS, fall back to LookupKey and
+ // validate the bucket layout client-side as before.
+ boolean omRejectsObs = proxy.getOmVersion()
Review Comment:
The fallback branch (OM older than `LOOKUP_FILE_REJECTS_OBS`) isn't covered by any test. All new integration tests run against a current OM, so a reversed comparison here, or a later removal of the `getBucket` call below, would still pass CI while silently letting OBS buckets through during a rolling upgrade.
Could you add a unit test in `ozonefs-common` (similar to `TestBasicRootedOzoneClientAdapterHeadOp`, which already mocks `proxy.getOmVersion()`) that uses an older OM version and verifies that `getBucketDetails` is called, an OBS bucket is still rejected client-side, `lookupKey` is used rather than `lookupFile`, and no InfoVolume call is made?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15951. Reduce ofs getFileChecksum to a single OM RPC [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #11380:
URL: https://github.com/apache/ozone/pull/11380#discussion_r4212764812
##########
hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicRootedOzoneClientAdapterGetFileChecksum.java:
##########
@@ -0,0 +1,199 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.fs.ozone;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.lang.reflect.Field;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.scm.OzoneClientConfig;
+import org.apache.hadoop.ozone.OzoneManagerVersion;
+import org.apache.hadoop.ozone.client.ObjectStore;
+import org.apache.hadoop.ozone.client.OzoneBucket;
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.helpers.BucketLayout;
+import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
+import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for {@link BasicRootedOzoneClientAdapterImpl#getFileChecksum},
+ * covering the single-RPC LookupFile path added by HDDS-15951, the LookupKey
+ * fallback used when the OM predates
+ * {@link OzoneManagerVersion#LOOKUP_FILE_REJECTS_OBS}, and the mapping of OM
+ * result codes onto file system exceptions. Uses a partial mock so no OM
+ * connection is required.
+ */
+public class TestBasicRootedOzoneClientAdapterGetFileChecksum {
Review Comment:
IMO having test class for BasicRootedOzoneClientAdapter is redundant, but ok as long as it doesn't take much time to run.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org