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: [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]
