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:
Cache-blind: this scans with `iterator()` while the sibling lookups (`:719`,
`:760`) use cache-consulting `.get()`. Both write sites demote the null version
via `addCacheEntry` (`OMKeyCommitRequest:429`, `OMKeyRequest:1627`), so a read
in the pre-flush window returns `KEY_NOT_FOUND` for a version that exists.
Repro: seed via `addCacheEntry`, then `lookupKey(nullVersion=true)` throws
while `versionId=0` resolves it.
The null version is always `UNSET_VERSION_ID` (0), so the comment is wrong
and the scan is unneeded — resolve with `.get()`:
```java
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:
No `KEY_IS_DELETE_MARKER` case in `S3ErrorTable.translateResultCode` →
`default: INTERNAL_ERROR` (:244) → HTTP 500, not 405 (no 405 mapping exists in
s3gateway). This comment and its twin at `OMException:290` describe behavior
that isn't there. Add the 405 case, or drop the claim.
--
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]
