HDDS-16046. [Ozone versioning] [T4] Read / permanent delete by versionId + promotion - #10937
Open
symious wants to merge 18 commits into
Open
HDDS-16046. [Ozone versioning] [T4] Read / permanent delete by versionId + promotion#10937symious wants to merge 18 commits into
symious wants to merge 18 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…equest Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key names in OBJECT_STORE buckets contain '/' verbatim, so a '/' separator interleaves a key's versions with those of keys nested under it, breaking the single-seek promotion and the merged ListObjectVersions order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds VersionIdGenerator, the pluggable source of the id an object version is numbered with, and UniqueIdVersionIdGenerator as the cluster default. The id is proposed on the OM that received the request, in preExecute, so it travels in the replicated request and every OM applies a version that is already numbered. Nothing about it depends on the transaction carrying the write, or on OM being replicated by Ratis. The default numbers a version with the time it was written, through the scheme Ozone already uses for block local IDs: currentTimeMillis << 16 with a 16-bit counter separating ids proposed inside one millisecond. It needs no allocator state and no coordination, which is what makes it safe to read on any OM. The interface has one abstract method, generateVersionId(), plus a default versionIdFor(proposed, hasCurrentVersion) that lets a generator number some versions specially at apply time. The implementation is selected cluster-wide by ozone.om.versioning.version-id-generator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds VersionIdAllocator, which turns the id proposed for a version into the id it is applied with. versionedKeyTable orders a key's versions by Long.MAX_VALUE - versionId, so the ids of one key have to increase in the order the versions were written. A proposal is a clock reading and cannot promise that: ids proposed inside one millisecond can exhaust the counter separating them, and a leader change onto a lagging clock proposes a lower value. So a proposal is a floor. The applied id is the later of it and the id after the key's current version, which the write path already holds - no read of its own, no global state, and identical on every OM. Under a clock regression an affected key's ids climb by one until proposals overtake them again: the versions stay ordered and only the id's reading as a time degrades. propose() runs in preExecute on the OM that received the request; allocate() runs under the write's lock on every OM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds PinnedFirstVersionIdGenerator, which numbers versions like the default except that a key's first version takes FIRST_VERSION_ID, so it can be referenced without listing the key's versions first. Whether the key already has a version is not known when the id is proposed, so the generator decides it in versionIdFor, under the write's lock, from the current version the allocator was handed. The sentinel is 1: below every proposed id, so a pinned version sorts at the old end of the key in versionedKeyTable, and above the unset value a pre-versioning record carries. It says nothing about the null version, which carries a proposed id like any other and is marked by isNullVersion. Known trade-off: once every version of a key has been permanently deleted, a recreated key takes the sentinel again, so an external reference to the first version resolves to the new content. The generator is off by default and selected by ozone.om.versioning.version-id-generator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On a versioning-enabled bucket a commit no longer reclaims the version it overwrites: the previous current version moves to the versionedKeyTable and the new record becomes current, in one WriteBatch. A record written before versioning was enabled carries no versionId and becomes the key's null version. The new version is numbered from KeyArgs.proposedVersionId, which preExecute stamps into the request on the OM that received it, so every OM applies the same id and nothing is generated during apply. The allocator raises the proposal to come after the key's current version when it does not already. An hsync re-commit keeps updating the version it opened, so it keeps its versionId and moves nothing. Both reclaim branches now depend on the versioning status rather than on the legacy isVersionEnabled flag being kept in sync with it, so dropping that sync cannot strand a version record by reclaiming the blocks it still refers to. S3MultipartUploadCompleteRequest is guarded the same way; it does not yet record a version for the key it supersedes, so an MPU overwrite on a versioned bucket leaks those blocks until T5 lands. OMKeyCommitRequestWithFSO is left alone: isS3VersioningEnabled() requires the OBJECT_STORE layout, so the check is structurally false there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A delete without a versionId on a versioning-enabled bucket removes no data: a delete marker becomes the key's current version and the version it supersedes moves to the versionedKeyTable. As in S3, the marker is inserted even when the key does not exist. The marker is a version, so its id comes from KeyArgs.proposedVersionId the same way a commit's does, proposed in preExecute and raised to come after the key's current version at apply time. The failure response declares the same tables as the successful one: the double buffer cleans the table cache from the response's CleanupTableInfo, so a marker request that failed after touching the versionedKeyTable cache would otherwise leave an entry behind that is in no DB and never cleaned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A batch delete is the same operation over more keys, but it goes through a request of its own and that one still hard-deleted on a versioned bucket: the current version's blocks were reclaimed and the versions underneath were left in the versionedKeyTable with nothing in the keyTable above them. Those versions then read as absent, hold quota, and cannot be promoted, because promotion only runs when a version is deleted by id. The gap opens with T3.1, where the two tables first diverge, so it is closed here rather than later. It is reachable today: the S3 gateway wires DeleteObjects straight to this request, and so does the client's deleteKeys API. Rather than write a second marker implementation, the one T3.2 added moves to OMKeyRequest and returns what it changed, so the single-key and batch requests build their own responses from the same insertion. One proposed versionId covers a batch: an id only has to increase within one key, and the applying OM raises it per key against that key's own current version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rich7420
reviewed
Aug 21, 2026
Comment on lines
+737
to
+752
| // 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; |
Contributor
There was a problem hiding this 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():
return metadataManager.getVersionedKeyTable().get(metadataManager
.getVersionedOzoneKey(volumeName, bucketName, keyName,
VersionIdGenerator.UNSET_VERSION_ID));(needs the VersionIdGenerator import)
Contributor
Author
There was a problem hiding this comment.
@rich7420 Thank you for the review.
The cache-blindness is real — fixed.
But the null version isn't always UNSET_VERSION_ID: a record written while versioning was SUSPENDED carries a normally allocated id plus isNullVersion (setNullVersion(suspendedWrite) in OMKeyCommitRequest), and keeps that id when it's later demoted. .get(UNSET_VERSION_ID) would miss it — TestKeyManagerUnit#testLookupKeyByVersionId seeds the null version at id 10.
So the search by attribute stays; it just has to consult the cache too. Now reuses the cache-aware search the write paths already had, extracted into NoncurrentVersions so there's one copy.
Two regression tests added.
| 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. |
Contributor
There was a problem hiding this 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.
Contributor
Author
There was a problem hiding this comment.
Right, there's no 405 in s3gateway at all yet. The endpoints are a later task in this chain, and a translateResultCode case alone wouldn't be enough (S3 also wants x-amz-delete-marker: true), so I've dropped the claim instead: the comments now say the code is kept distinct so the gateway can answer 405, with the mapping landing alongside the endpoints.
Contributor
|
@symious thanks for the patch! |
GET/HEAD/lookup can address one version of a key instead of its current one: KeyArgs carries either a versionId or nullVersion, at most one. A versionId naming the current version is answered from the keyTable with no versionedKeyTable read; anything else is a point lookup there. The null version is found by its isNullVersion attribute rather than by id, because it carries a normally proposed id like any other version, so no dbKey addresses it. That search reads the table cache as well as the DB: Table.iterator() goes straight to RocksDB, and a version demoted by a transaction the double buffer has not flushed yet lives only in the cache, so an iterator-only scan would report a version that exists as missing while the point lookups beside it, which do consult the cache, resolve it. A read that lands on a current delete marker is a plain not-found, while one that names a marker by id - current or not - is KEY_IS_DELETE_MARKER. S3 answers such a read with 405 rather than 404, which is why the two conditions stay distinct; the gateway mapping arrives with the endpoints. Only OBJECT_STORE buckets can hold versions, so addressing a version on any other layout is NOT_SUPPORTED_OPERATION. The check is on the layout rather than on isFileSystemOptimized(), because LEGACY reaches the same lookup path as OBJECT_STORE and would otherwise scan the versionedKeyTable and report the key not found. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DELETE ?versionId= is the only delete that destroys data on a versioned bucket: the version leaves the versionedKeyTable and its blocks go to the deletedTable, which stays the single path through which version blocks are reclaimed. The null slot is addressed by attribute, so it is found by the same bounded prefix scan the read path uses. Addressing the current version is rejected for now: removing it has to promote the next-newest version to keep the keyTable authoritative, which T4.3 adds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
keyTable holds the current version of every key that still has one, so removing the current version has to hand the place over: one seek on the key's version prefix yields the newest remaining version, which moves back into the keyTable in the same WriteBatch as the delete. The record travels unchanged - promotion is positional, and a version keeps the identity it was created with. When no version survives, the key disappears entirely. Deleting a current delete marker this way is exactly S3's restore-an-object flow: the version the marker superseded becomes current again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Please only review commits starts with "T4".
This ticket includes the following tasks.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16046
How was this patch tested?
unit test.