chihsuan commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r3595856989
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -462,6 +478,22 @@ public Builder addAcl(OzoneAcl ozoneAcl) {
public Builder setIsVersionEnabled(boolean versionFlag) {
this.isVersionEnabled = versionFlag;
+ // Keep versioningStatus in sync for callers that only know the legacy
+ // flag; an explicitly SUSPENDED status is preserved on disable.
+ if (versionFlag) {
+ this.versioningStatus = BucketVersioningStatus.ENABLED;
+ } else if (versioningStatus != BucketVersioningStatus.SUSPENDED) {
+ this.versioningStatus = BucketVersioningStatus.UNVERSIONED;
+ }
+ return this;
+ }
+
+ /** No-op when status is null (e.g. records without the new field). */
+ public Builder setVersioningStatus(BucketVersioningStatus status) {
Review Comment:
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:
https://github.com/apache/ozone/blob/b981afbf89e28e8fb182ec9ba4e1025b9e7ea405/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3059-L3072
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?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java:
##########
@@ -112,6 +112,15 @@ public final class OmKeyInfo extends WithParentObjectId
// been modified.
private final Long expectedDataGeneration;
+ // S3-compatible object versioning. versionId is assigned once from the
+ // committing transaction's index when a version is created, then frozen;
+ // 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;
Review Comment:
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.
https://github.com/apache/ozone/blob/f74c3318223dd79ef69b708eb000a38defee22f1/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java#L1004-L1017
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -108,6 +114,14 @@ public Boolean getIsVersionEnabled() {
return isVersionEnabled;
}
+ /**
+ * Returns the requested versioning status, or null if not being changed.
+ * @return BucketVersioningStatus
+ */
+ public BucketVersioningStatus getVersioningStatus() {
Review Comment:
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?
https://github.com/apache/ozone/blob/f74c3318223dd79ef69b708eb000a38defee22f1/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java#L199-L206
--
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]
