rich7420 commented on code in PR #10937:
URL: https://github.com/apache/ozone/pull/10937#discussion_r3830010822
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -697,6 +719,48 @@ private OmKeyInfo getOmKeyInfo(String volumeName, String
bucketName,
.get(keyBytes);
}
+ /**
+ * Resolves the version addressed by {@code args} for a key whose current
+ * version is {@code current}. The current version is checked first, so a
+ * request naming the current version costs no extra read; otherwise the
+ * version is looked up in the versionedKeyTable.
+ *
+ * @param current the key's current version, or null when the key has none
+ * @return the addressed version, or null when it does not exist
+ */
+ private OmKeyInfo getAddressedVersion(OmKeyArgs args, String volumeName,
+ String bucketName, String keyName, OmKeyInfo current) throws IOException
{
+ if (args.isNullVersion()) {
+ if (current != null && current.isNullVersion()) {
+ return current;
+ }
+ // The null version carries a normally generated versionId, so it can
only
+ // be found by scanning the key's versions. The scan is bounded by the
+ // number of versions the key has and stops at the first match, since a
key
+ // has at most one null version.
+ String prefix = metadataManager
+ .getVersionedOzoneKeyPrefix(volumeName, bucketName, keyName);
+ try (Table.KeyValueIterator<String, OmKeyInfo> versions =
+ metadataManager.getVersionedKeyTable().iterator(prefix)) {
+ while (versions.hasNext()) {
+ OmKeyInfo version = versions.next().getValue();
+ if (version.isNullVersion()) {
+ return version;
+ }
+ }
+ }
+ return null;
Review Comment:
This resolves the null version with a cache-blind iterator scan, while every
sibling lookup (`getOmKeyInfo` at :719 and the `versionId` branch at :760) uses
a cache-consulting `.get()`. Both write sites demote the null version via
`addCacheEntry` (`OMKeyCommitRequest:429-433`, `OMKeyRequest:1627-1632`), so a
null-version read in the pre-flush window returns `KEY_NOT_FOUND` for a version
that exists. Repro: seed the null version with `addCacheEntry`, then
`lookupKey(nullVersion=true)` throws `KEY_NOT_FOUND`, while the same entry
addressed by `versionId=0` resolves.
The null version is always stored with `UNSET_VERSION_ID` (0) — both write
sites set it — so the comment here ("normally generated versionId … can only be
found by scanning") is wrong and the scan is unnecessary. Resolve it with a
`.get()`, like the `versionId` branch:
```java
// The null version is always stored with UNSET_VERSION_ID, so resolve
it
// with a cache-consulting get() rather than a cache-blind iterator
scan.
return metadataManager.getVersionedKeyTable().get(metadataManager
.getVersionedOzoneKey(volumeName, bucketName, keyName,
VersionIdGenerator.UNSET_VERSION_ID));
```
(needs the `VersionIdGenerator` import.)
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -609,6 +609,10 @@ enum Status {
LIFECYCLE_CONFIGURATION_NOT_FOUND = 102;
UPDATE_ID_NOT_MATCH = 103;
+
+ // The addressed version exists but is a delete marker. Distinct from
+ // KEY_NOT_FOUND: the S3 Gateway maps it to 405, not 404.
Review Comment:
`S3ErrorTable.translateResultCode` has no `KEY_IS_DELETE_MARKER` case, so it
falls to `default: return INTERNAL_ERROR` (:244-245) → HTTP 500, not 405; there
is no 405 / `METHOD_NOT_ALLOWED` mapping anywhere in s3gateway. This comment
(and its twin at `OMException.java:290`) describes behavior that doesn't exist.
Either add the `KEY_IS_DELETE_MARKER` → 405 case now, or drop the "maps it to
405" claim and reference the follow-up that wires it up.
--
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]
