Skip to content

HDDS-15879. Ozone Versioning Metadata foundation#10781

Open
symious wants to merge 6 commits into
apache:HDDS-15728from
symious:HDDS-15879
Open

HDDS-15879. Ozone Versioning Metadata foundation#10781
symious wants to merge 6 commits into
apache:HDDS-15728from
symious:HDDS-15879

Conversation

@symious

@symious symious commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This is the first basic task of Ozone versioning, including the following points

  • bucket proto three-state enum
  • set-property state machine
  • OmKeyInfo proto extension
  • versionedKeyTableColumnFamily
  • layout feature gate

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15879

How was this patch tested?

unit test.

symious and others added 5 commits July 16, 2026 11:06
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>

@chihsuan chihsuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean foundation work! @symious I verified that the five commits map nicely to the design doc. Left a few small comments below.

}

/** No-op when status is null (e.g. records without the new field). */
public Builder setVersioningStatus(BucketVersioningStatus status) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work keeping the two fields in sync here! One spot that looks left behind:

OzoneManager#getBucketInfo copies only the legacy flag when resolving a link bucket:

return bucketInfo.toBuilder()
.setDefaultReplicationConfig(
realBucket.getDefaultReplicationConfig())
.setIsVersionEnabled(realBucket.getIsVersionEnabled())
.setStorageType(realBucket.getStorageType())
.setQuotaInBytes(realBucket.getQuotaInBytes())
.setQuotaInNamespace(realBucket.getQuotaInNamespace())
.setUsedBytes(realBucket.getUsedBytes())
.setSnapshotUsedBytes(realBucket.getSnapshotUsedBytes())
.setSnapshotUsedNamespace(realBucket.getSnapshotUsedNamespace())
.setUsedNamespace(realBucket.getUsedNamespace())
.addAllMetadata(realBucket.getMetadata())
.setBucketLayout(realBucket.getBucketLayout())
.build();

Since SUSPENDED maps to flag=false, a suspended bucket read through a link would come back as UNVERSIONED. Should we also add .setVersioningStatus(realBucket.getVersioningStatus()) there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, Updated, PTAL.

* Returns the requested versioning status, or null if not being changed.
* @return BucketVersioningStatus
*/
public BucketVersioningStatus getVersioningStatus() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny one: toAuditMap() still only records IS_VERSION_ENABLED, so a status change via the new field would show up as isVersionEnabled=null in the audit log. Worth adding a versioningStatus entry while we're here?

public Map<String, String> toAuditMap() {
Map<String, String> auditMap = new LinkedHashMap<>();
auditMap.put(OzoneConsts.VOLUME, this.volumeName);
auditMap.put(OzoneConsts.BUCKET, this.bucketName);
auditMap.put(OzoneConsts.GDPR_FLAG,
getMetadata().get(OzoneConsts.GDPR_FLAG));
auditMap.put(OzoneConsts.IS_VERSION_ENABLED,
String.valueOf(this.isVersionEnabled));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL.

// absent on records that predate versioning support (treated as the null
// version). A delete marker has isDeleteMarker set and no data blocks.
// isNullVersion marks the single overwritable "null version" slot per key.
private final Long versionId;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add these three fields to isKeyInfoSame() (and toString())? Right now two records differing only in versionId, or a delete marker vs. a regular key, compare as equal. Looks like the follow-up tasks will rely on OmKeyInfo equality in tests; having this in the foundation would prevent false-positive assertions later. OmBucketInfo already includes the new status in both, so this would keep the two helpers consistent.

public boolean isKeyInfoSame(OmKeyInfo omKeyInfo, boolean checkPath,
boolean checkKeyLocationVersions,
boolean checkModificationTime,
boolean checkUpdateID,
boolean checkOwnerName) {
boolean isEqual = dataSize == omKeyInfo.dataSize &&
creationTime == omKeyInfo.creationTime &&
volumeName.equals(omKeyInfo.volumeName) &&
bucketName.equals(omKeyInfo.bucketName) &&
replicationConfig.equals(omKeyInfo.replicationConfig) &&
Objects.equals(getMetadata(), omKeyInfo.getMetadata()) &&
Objects.equals(acls, omKeyInfo.acls) &&
Objects.equals(getTags(), omKeyInfo.getTags()) &&
getObjectID() == omKeyInfo.getObjectID();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, Updated, PTAL.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants