You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "symious (via GitHub)" <gi...@apache.org> on 2026/07/16 06:25:29 UTC
[PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
symious opened a new pull request, #10781:
URL: https://github.com/apache/ozone/pull/10781
## 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.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r3600097980
##########
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:
Nice catch, Updated, PTAL.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition, and OmBucketInfo too?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition, and OmBucketInfo too?
If we want to have a isVersionEnabled check(unset & missing vs enabled&disabled, I prefer we implement a isVersionEnabled() function instead of have two fields which one has dependency or precedence to the other.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268669070
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketVersioningStatus.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.helpers;
+
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.BucketVersioningStatusProto;
+
+/**
+ * S3-compatible bucket versioning state machine:
+ * UNVERSIONED -> ENABLED <-> SUSPENDED.
+ * Once versioning has been enabled, a bucket can never return to UNVERSIONED.
+ */
+public enum BucketVersioningStatus {
+ UNVERSIONED,
+ ENABLED,
+ SUSPENDED;
+
+ public static BucketVersioningStatus fromProto(BucketVersioningStatusProto proto) {
+ switch (proto) {
+ case VERSIONING_ENABLED:
+ return ENABLED;
+ case VERSIONING_SUSPENDED:
+ return SUSPENDED;
+ case UNVERSIONED:
+ default:
+ return UNVERSIONED;
+ }
+ }
+
+ public BucketVersioningStatusProto toProto() {
+ switch (this) {
+ case ENABLED:
+ return BucketVersioningStatusProto.VERSIONING_ENABLED;
+ case SUSPENDED:
+ return BucketVersioningStatusProto.VERSIONING_SUSPENDED;
+ default:
+ return BucketVersioningStatusProto.UNVERSIONED;
+ }
+ }
+
+ /** The legacy isVersionEnabled flag value kept in sync with this status. */
+ public boolean toVersionEnabledFlag() {
Review Comment:
Can we change this to isVersionEnabled?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376302948
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmBucketInfo.java:
##########
@@ -54,6 +55,77 @@ public void protobufConversion() {
OmBucketInfo.getFromProtobuf(bucket.getProtobuf()));
}
+ @Test
+ public void deprecatedVersionFlagIsIgnoredWhenRead() {
+ // A record carrying only the deprecated flag has no versioning status and
+ // reads as UNVERSIONED, whatever the flag says.
+ OzoneManagerProtocolProtos.BucketInfo oldRecord =
+ OzoneManagerProtocolProtos.BucketInfo.newBuilder()
+ .setVolumeName("vol1")
+ .setBucketName("bucket")
+ .setIsVersionEnabled(true)
Review Comment:
I think IsVersionEnabled true is mapped to BucketVersioningStatus.ENABLED?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4269097315
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -101,6 +101,17 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// FSO and LEGACY buckets are not strictly bound to S3 naming semantics.
OmUtils.validateBucketName(bucketInfo.getBucketName(), strict);
+ // S3 has no way to create a bucket that is already in a versioning
Review Comment:
Is this comment generated by AI? The comment from AI is really lengthy, and not as clear as comment from dev, sometime even confusing.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4269049641
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java:
##########
@@ -89,6 +89,7 @@
* | Column Family | Mapping |
* |----------------------------------------------------------------------------------|
* | keyTable | /volume/bucket/key :- KeyInfo |
+ * | versionedKeyTable | /volume/bucket/key\0revVersionId :- KeyInfo |
Review Comment:
Not a this patch's problem here. But if "/" is allowed in key name, then openKeyTable and multipartInfoTable might have problem when "/" is used.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition, and OmBucketInfo too?
If we want to have a isVersionEnabled check, I prefer we implement a isVersionEnabled() function instead of have two fields which one has dependency or precedence to the other.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r3838825982
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java:
##########
@@ -376,4 +402,27 @@ public static OMRequest disallowSetBucketPropertyWithECReplicationConfig(
}
return req;
}
+
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.SetBucketProperty
+ )
+ public static OMRequest disallowSetBucketPropertyWithVersioningStatus(
Review Comment:
This gate covers `SetBucketProperty`, but `CreateBucket` carries the same `BucketInfo.versioningStatus` and persists it (`OmBucketInfo.getFromProtobuf`) with no matching `OBJECT_VERSIONING` validator — so a pre-finalized cluster would accept a `CreateBucket` that sets it, bypassing this contract (and the create path runs no `canTransitionTo`). Not triggerable yet since no client sends the field. Worth mirroring this validator on `Type.CreateBucket`, or a TODO + follow-up Jira.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4386754060
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java:
##########
@@ -388,4 +396,28 @@ public static OMRequest disallowSetBucketPropertyWithECReplicationConfig(
}
return req;
}
+
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.SetBucketProperty
+ )
+ public static OMRequest disallowSetBucketPropertyWithVersioningStatus(
+ OMRequest req, ValidationContext ctx) throws OMException {
+ if (!ctx.versionManager().isAllowed(OzoneManagerVersion.S3_OBJECT_VERSIONING)) {
+ SetBucketPropertyRequest propReq =
+ req.getSetBucketPropertyRequest();
+ // The deprecated isVersionEnabled flag also maps to a versioning status.
+ if (propReq.hasBucketArgs()
+ && (propReq.getBucketArgs().hasVersioningStatus()
+ || propReq.getBucketArgs().hasIsVersionEnabled())) {
Review Comment:
I think most the new comments are related to the handling of `isVersionEnabled` and `versionStatus` after we decide the deperacate `isVersionEnabled` in this commit.
As an earlier version is able to handle these two properties, how about we revert back to the earlier version and do the deperacate in another seperate ticket?
Or we do the hard deperacate in this ticket and rejects all the `isVersionEnable` request here?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -180,6 +180,11 @@ public final class OzoneConsts {
public static final String OM_KEY_PREFIX = "/";
public static final String DOUBLE_SLASH_OM_KEY_PREFIX = "//";
+ /**
+ * Separates the key name from the versionId in versionedKeyTable DB keys.
+ * Not '/', since key names may contain it.
+ */
+ public static final String OM_VERSIONED_KEY_SEPARATOR = "\0";
Review Comment:
‘\0’ is used on purpose: as the smallest byte, it keeps a key’s versions contiguous and sorted the same way as the key names in keyTable, which ListObjectVersions relies on when merging the two tables. With ‘/’, the versions of “a” would interleave with keys nested under it such as “a/b”, and ‘/’ can’t be disallowed in key names, because S3 prefixes and the FSO directory tree are built on it. To make sure ‘\0’ never appears in a key name, we can reject it either with a separate check that is always on, or by adding it to KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX, which only applies when ozone.om.keyname.character.check.enabled is true (false by default). What do you think?
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
Review Comment:
createBucket now rejects versioning regardless of the OM version, since versioning can only be set after the bucket is created. The case you point at does exist in setBucketVersioning, though: an older OM would silently ignore versioningStatus. I added the same version check there, so the client fails fast when OM doesn’t support S3_OBJECT_VERSIONING.
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmBucketInfo.java:
##########
@@ -54,6 +55,77 @@ public void protobufConversion() {
OmBucketInfo.getFromProtobuf(bucket.getProtobuf()));
}
+ @Test
+ public void deprecatedVersionFlagIsIgnoredWhenRead() {
+ // A record carrying only the deprecated flag has no versioning status and
+ // reads as UNVERSIONED, whatever the flag says.
+ OzoneManagerProtocolProtos.BucketInfo oldRecord =
+ OzoneManagerProtocolProtos.BucketInfo.newBuilder()
+ .setVolumeName("vol1")
+ .setBucketName("bucket")
+ .setIsVersionEnabled(true)
Review Comment:
It is confusing because the same flag name is used both in the stored bucket record (BucketInfo) and in the SetBucketProperty request (BucketArgs).
For simplicity, we can also ignore the setter of `isVersionEnabled`, so that we can only focus on status, what do you think?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -682,7 +734,10 @@ public static Builder builderFromProtobuf(BucketInfo bucketInfo,
.setBucketName(bucketInfo.getBucketName())
.setAcls(bucketInfo.getAclsList().stream().map(
OzoneAcl::fromProtobuf).collect(Collectors.toList()))
- .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+ // The deprecated isVersionEnabled flag is ignored.
+ .setVersioningStatus(bucketInfo.hasVersioningStatus()
+ ? BucketVersioningStatus.fromProto(bucketInfo.getVersioningStatus())
+ : null)
Review Comment:
Since isVersionEnabled is deprecated, versioningStatus is the only source of truth here, and a record without it reads as UNVERSIONED.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -914,7 +928,8 @@ message DataEncryptionKeyProto {
message BucketArgs {
required string volumeName = 1;
required string bucketName = 2;
- optional bool isVersionEnabled = 5;
+ // Deprecated, use versioningStatus. true means ENABLED, false SUSPENDED.
Review Comment:
Mapping false to UNVERSIONED alone would not work for a bucket that is currently ENABLED, since it can’t go back to UNVERSIONED. I updated the comment to match the new behavior: false suspends an ENABLED bucket and leaves an UNVERSIONED one unchanged.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -256,8 +268,20 @@ public Builder setBucketName(String bucket) {
return this;
}
+ /**
+ * Maps the legacy flag onto the versioning status: true enables
+ * versioning, false suspends it.
+ * @deprecated use {@link #setVersioningStatus(BucketVersioningStatus)}.
+ */
+ @Deprecated
public Builder setIsVersionEnabled(Boolean versionFlag) {
- this.isVersionEnabled = versionFlag;
+ this.versioningStatus = versionFlag == null ? null : versionFlag
Review Comment:
`isVersionEnabled=false` now leaves a never-versioned bucket UNVERSIONED and only suspends a bucket that is currently ENABLED, since ENABLED can’t go back to UNVERSIONED.
To make it more simple, how about just ignore isVersionEnabled, and only handle `versionStatus`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4309812611
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Agreed, updated, PTAL.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #10781:
URL: https://github.com/apache/ozone/pull/10781#issuecomment-5024611817
@yandrey321
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4269097315
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -101,6 +101,17 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// FSO and LEGACY buckets are not strictly bound to S3 naming semantics.
OmUtils.validateBucketName(bucketInfo.getBucketName(), strict);
+ // S3 has no way to create a bucket that is already in a versioning
Review Comment:
Is this comment generated by AI? The comments from AI these days are really lengthy, and not as clear as comment from dev, sometime even confusing.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376216479
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -682,7 +734,10 @@ public static Builder builderFromProtobuf(BucketInfo bucketInfo,
.setBucketName(bucketInfo.getBucketName())
.setAcls(bucketInfo.getAclsList().stream().map(
OzoneAcl::fromProtobuf).collect(Collectors.toList()))
- .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+ // The deprecated isVersionEnabled flag is ignored.
+ .setVersioningStatus(bucketInfo.hasVersioningStatus()
+ ? BucketVersioningStatus.fromProto(bucketInfo.getVersioningStatus())
+ : null)
Review Comment:
If bucketInfo.hasVersioningStatus() is null, shall we also check IsVersionEnabled(), like what is done in OmBucketArgs?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r3840446806
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java:
##########
@@ -376,4 +402,27 @@ public static OMRequest disallowSetBucketPropertyWithECReplicationConfig(
}
return req;
}
+
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.SetBucketProperty
+ )
+ public static OMRequest disallowSetBucketPropertyWithVersioningStatus(
Review Comment:
Good catch. Following S3 here: CreateBucket has no versioning parameter, versioning is only set on an existing bucket via PutBucketVersioning. So instead of the layout gate, T1.2 now rejects any CreateBucket carrying versioningStatus outright with INVALID_REQUEST. That covers post-finalization too, so I dropped the mirrored validator from T1.5.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on PR #10781:
URL: https://github.com/apache/ozone/pull/10781#issuecomment-6359969143
Raised https://github.com/apache/ozone/pull/11708 for the failed CI test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376015686
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -256,8 +268,20 @@ public Builder setBucketName(String bucket) {
return this;
}
+ /**
+ * Maps the legacy flag onto the versioning status: true enables
+ * versioning, false suspends it.
+ * @deprecated use {@link #setVersioningStatus(BucketVersioningStatus)}.
+ */
+ @Deprecated
public Builder setIsVersionEnabled(Boolean versionFlag) {
- this.isVersionEnabled = versionFlag;
+ this.versioningStatus = versionFlag == null ? null : versionFlag
Review Comment:
This is one example bucket DB info. It looks like default bucket isVersionEnabled is "false", which is better mapped to BucketVersioningStatus.UNVERSIONED instead of "BucketVersioningStatus.SUSPENDED", @symious what do you think?
```
{ "/s3v/lance": {
"metadata" : { },
"objectID" : -9223372036854775552,
"updateID" : 1,
"volumeName" : "s3v",
"bucketName" : "lance",
"acls" : [ {
"type" : "USER",
"name" : "sammi",
"aclScope" : "ACCESS"
}, {
"type" : "GROUP",
"name" : "staff",
"aclScope" : "ACCESS"
} ],
"isVersionEnabled" : false,
"storageType" : "DISK",
"creationTime" : 1792068900430,
"modificationTime" : 1792068900430,
"usedBytes" : 544269232,
"usedNamespace" : 13,
"quotaInBytes" : -1,
"quotaInNamespace" : -1,
"snapshotUsedBytes" : 0,
"snapshotUsedNamespace" : 0,
"bucketLayout" : "FILE_SYSTEM_OPTIMIZED",
"owner" : "sammi",
"tags" : { }
}
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376403368
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
Review Comment:
It's better we have similar check, like EC, to reject when isVersionEnabled is true, if OM doesn't support "S3_OBJECT_VERSIONING".
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4309811589
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java:
##########
@@ -101,6 +101,17 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
// FSO and LEGACY buckets are not strictly bound to S3 naming semantics.
OmUtils.validateBucketName(bucketInfo.getBucketName(), strict);
+ // S3 has no way to create a bucket that is already in a versioning
Review Comment:
Updated, PTAL.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java:
##########
@@ -89,6 +89,7 @@
* | Column Family | Mapping |
* |----------------------------------------------------------------------------------|
* | keyTable | /volume/bucket/key :- KeyInfo |
+ * | versionedKeyTable | /volume/bucket/key\0revVersionId :- KeyInfo |
Review Comment:
Agreed, better have another ticket to fix this problem.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/BucketVersioningStatus.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.helpers;
+
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.BucketVersioningStatusProto;
+
+/**
+ * S3-compatible bucket versioning state machine:
+ * UNVERSIONED -> ENABLED <-> SUSPENDED.
+ * Once versioning has been enabled, a bucket can never return to UNVERSIONED.
+ */
+public enum BucketVersioningStatus {
+ UNVERSIONED,
+ ENABLED,
+ SUSPENDED;
+
+ public static BucketVersioningStatus fromProto(BucketVersioningStatusProto proto) {
+ switch (proto) {
+ case VERSIONING_ENABLED:
+ return ENABLED;
+ case VERSIONING_SUSPENDED:
+ return SUSPENDED;
+ case UNVERSIONED:
+ default:
+ return UNVERSIONED;
+ }
+ }
+
+ public BucketVersioningStatusProto toProto() {
+ switch (this) {
+ case ENABLED:
+ return BucketVersioningStatusProto.VERSIONING_ENABLED;
+ case SUSPENDED:
+ return BucketVersioningStatusProto.VERSIONING_SUSPENDED;
+ default:
+ return BucketVersioningStatusProto.UNVERSIONED;
+ }
+ }
+
+ /** The legacy isVersionEnabled flag value kept in sync with this status. */
+ public boolean toVersionEnabledFlag() {
Review Comment:
This method is removed in the latest commit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376666276
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -180,6 +180,11 @@ public final class OzoneConsts {
public static final String OM_KEY_PREFIX = "/";
public static final String DOUBLE_SLASH_OM_KEY_PREFIX = "//";
+ /**
+ * Separates the key name from the versionId in versionedKeyTable DB keys.
+ * Not '/', since key names may contain it.
+ */
+ public static final String OM_VERSIONED_KEY_SEPARATOR = "\0";
Review Comment:
I'm not very sure that "\0" is safe or not.
According to AWS S3 doc, https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines, "ASCII character ranges 00–1F hex (0–31 decimal) and 7F (127 decimal) " is "Characters that might require special handling". "/" falls to this category too.
And there is also a "Characters to avoid" list, which looks like a better choice.
In the meanwhile, key name check in Ozone is controlled by "ozone.om.keyname.character.check.enabled" which default is false.
If it's enabled, it verify the key name against following, which includes the "Characters to avoid" list.
```
/**
* Block key name as illegal characters
*
* This regular expression is used to check if key name
* contains illegal characters when creating/renaming key.
*
* Avoid the following characters in a key name:
* {@literal "\", "{", "}", "<", ">", "^", "%", "~", "#", "|", "`", "[", "]"}, Quotation
* marks and Non-printable ASCII characters (128–255 decimal characters).
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
*/
public static final Pattern KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX =
Pattern.compile("^[^\\\\{}<>^%~#|`\\[\\]\"\\x80-\\xff]+$");
```
So basically, both "/" and "\0" are allowed whether "ozone.om.keyname.character.check.enabled" is true or not.
Maybe we should choose some character from the "Characters to avoid" list, make "ozone.om.keyname.character.check.enabled" default to true.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376216479
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -682,7 +734,10 @@ public static Builder builderFromProtobuf(BucketInfo bucketInfo,
.setBucketName(bucketInfo.getBucketName())
.setAcls(bucketInfo.getAclsList().stream().map(
OzoneAcl::fromProtobuf).collect(Collectors.toList()))
- .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+ // The deprecated isVersionEnabled flag is ignored.
+ .setVersioningStatus(bucketInfo.hasVersioningStatus()
+ ? BucketVersioningStatus.fromProto(bucketInfo.getVersioningStatus())
+ : null)
Review Comment:
If bucketInfo.hasVersioningStatus() is null, shall we also check getIsVersionEnabled(), like what is done in OmBucketArgs? BTW, isVersionEnabled is a required field, so has() will always return true.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376216479
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -682,7 +734,10 @@ public static Builder builderFromProtobuf(BucketInfo bucketInfo,
.setBucketName(bucketInfo.getBucketName())
.setAcls(bucketInfo.getAclsList().stream().map(
OzoneAcl::fromProtobuf).collect(Collectors.toList()))
- .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+ // The deprecated isVersionEnabled flag is ignored.
+ .setVersioningStatus(bucketInfo.hasVersioningStatus()
+ ? BucketVersioningStatus.fromProto(bucketInfo.getVersioningStatus())
+ : null)
Review Comment:
If bucketInfo.hasVersioningStatus() is null, shall we also check hasIsVersionEnabled(), like what is done in OmBucketArgs?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376216479
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketInfo.java:
##########
@@ -682,7 +734,10 @@ public static Builder builderFromProtobuf(BucketInfo bucketInfo,
.setBucketName(bucketInfo.getBucketName())
.setAcls(bucketInfo.getAclsList().stream().map(
OzoneAcl::fromProtobuf).collect(Collectors.toList()))
- .setIsVersionEnabled(bucketInfo.getIsVersionEnabled())
+ // The deprecated isVersionEnabled flag is ignored.
+ .setVersioningStatus(bucketInfo.hasVersioningStatus()
+ ? BucketVersioningStatus.fromProto(bucketInfo.getVersioningStatus())
+ : null)
Review Comment:
If bucketInfo.hasVersioningStatus() is null, shall we also check hasIsVersionEnabled(), like what is done in OmKeyArgs?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on PR #10781:
URL: https://github.com/apache/ozone/pull/10781#issuecomment-5029513165
@chungen0126 OEP file here: https://github.com/apache/ozone/pull/10822
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376666276
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -180,6 +180,11 @@ public final class OzoneConsts {
public static final String OM_KEY_PREFIX = "/";
public static final String DOUBLE_SLASH_OM_KEY_PREFIX = "//";
+ /**
+ * Separates the key name from the versionId in versionedKeyTable DB keys.
+ * Not '/', since key names may contain it.
+ */
+ public static final String OM_VERSIONED_KEY_SEPARATOR = "\0";
Review Comment:
I'm not very sure that "\0" is safe or not.
According to AWS S3 doc, https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines, "ASCII character ranges 00–1F hex (0–31 decimal) and 7F (127 decimal) " is "Characters that might require special handling". "/" falls to this category too.
And there is also a "Characters to avoid" list, which looks like a better choice.
In the meanwhile, key name check in Ozone is controlled by "ozone.om.keyname.character.check.enabled" which default is false.
If it's enabled, it verify the key name against following, which includes the "Characters to avoid" list.
```
/**
* Block key name as illegal characters
*
* This regular expression is used to check if key name
* contains illegal characters when creating/renaming key.
*
* Avoid the following characters in a key name:
* {@literal "\", "{", "}", "<", ">", "^", "%", "~", "#", "|", "`", "[", "]"}, Quotation
* marks and Non-printable ASCII characters (128–255 decimal characters).
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
*/
public static final Pattern KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX =
Pattern.compile("^[^\\\\{}<>^%~#|`\\[\\]\"\\x80-\\xff]+$");
```
So basically, both "/" and "\0" are allowed whether "ozone.om.keyname.character.check.enabled" is true or not.
Maybe we should choose some character from the "Characters to avoid" list, make "ozone.om.keyname.character.check.enabled" default to true, at least for OBS bucket.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376774642
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java:
##########
@@ -388,4 +396,28 @@ public static OMRequest disallowSetBucketPropertyWithECReplicationConfig(
}
return req;
}
+
+ @RequestFeatureValidator(
+ conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,
+ processingPhase = RequestProcessingPhase.PRE_PROCESS,
+ requestType = Type.SetBucketProperty
+ )
+ public static OMRequest disallowSetBucketPropertyWithVersioningStatus(
+ OMRequest req, ValidationContext ctx) throws OMException {
+ if (!ctx.versionManager().isAllowed(OzoneManagerVersion.S3_OBJECT_VERSIONING)) {
+ SetBucketPropertyRequest propReq =
+ req.getSetBucketPropertyRequest();
+ // The deprecated isVersionEnabled flag also maps to a versioning status.
+ if (propReq.hasBucketArgs()
+ && (propReq.getBucketArgs().hasVersioningStatus()
+ || propReq.getBucketArgs().hasIsVersionEnabled())) {
Review Comment:
Will it be better to use getIsVersionEnabled() here instead of hasIsVersionEnabled() here, to be consistent with OMBucketCreateRequest check? So if IsVersionEnabled is set but it's false, then it will still succeed, for old clients.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition, and OmBucketInfo too, instead of support both fields correctly?
If we want to have a isVersionEnabled check(unset & missing vs enabled&disabled, I prefer we implement a isVersionEnabled() function instead of have two fields which one has dependency or precedence to the other.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "symious (via GitHub)" <gi...@apache.org>.
symious commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r3600097216
##########
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 catch, Updated, PTAL.
##########
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:
Updated, PTAL.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. Ozone Versioning Metadata foundation [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #10781:
URL: https://github.com/apache/ozone/pull/10781#issuecomment-5029430368
Thanks @symious for working on this! However, based on a recent community discussion, we’ve decided to deprecate Google Docs in favor of maintaining Markdown files via PRs (see https://github.com/apache/ozone/discussions/8284 ). Could you please convert the Google Doc into a Markdown file and submit it as a PR instead? Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4268528652
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmBucketArgs.java:
##########
@@ -44,6 +44,11 @@ public final class OmBucketArgs extends WithMetadata implements Auditable {
* Bucket Version flag.
*/
private final Boolean isVersionEnabled;
+ /**
Review Comment:
Can we make the isVersionEnabled deprecated on both this class and proto definition, and OmBucketInfo too, instead of support both fields with careful handling?
If we want to have a isVersionEnabled check(unset & missing vs enabled&disabled, I prefer we implement a isVersionEnabled() function instead of have two fields which one has dependency or precedence to the other.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376063066
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -914,7 +928,8 @@ message DataEncryptionKeyProto {
message BucketArgs {
required string volumeName = 1;
required string bucketName = 2;
- optional bool isVersionEnabled = 5;
+ // Deprecated, use versioningStatus. true means ENABLED, false SUSPENDED.
Review Comment:
"true means ENABLED, false SUSPENDED", will "true means ENABLED, false UNVERSIONED" be better?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376403368
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
Review Comment:
It's better we have similar check for object versioning, like EC.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15879. [Ozone Versioning] [T1] Metadata foundation [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10781:
URL: https://github.com/apache/ozone/pull/10781#discussion_r4376666276
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -180,6 +180,11 @@ public final class OzoneConsts {
public static final String OM_KEY_PREFIX = "/";
public static final String DOUBLE_SLASH_OM_KEY_PREFIX = "//";
+ /**
+ * Separates the key name from the versionId in versionedKeyTable DB keys.
+ * Not '/', since key names may contain it.
+ */
+ public static final String OM_VERSIONED_KEY_SEPARATOR = "\0";
Review Comment:
I'm not very sure that "\0" is safe or not.
According to AWS S3 doc, https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines, "ASCII character ranges 00–1F hex (0–31 decimal) and 7F (127 decimal) " is "Characters that might require special handling". "/" falls to this category too.
And there is also a "Characters to avoid" list, which looks like a better choice.
In the meanwhile, key name check in Ozone is controlled by "ozone.om.keyname.character.check.enabled" which default is false.
If it's enabled, it verify the key name against following, which includes the "Characters to avoid" list.
```
/**
* Block key name as illegal characters
*
* This regular expression is used to check if key name
* contains illegal characters when creating/renaming key.
*
* Avoid the following characters in a key name:
* {@literal "\", "{", "}", "<", ">", "^", "%", "~", "#", "|", "`", "[", "]"}, Quotation
* marks and Non-printable ASCII characters (128–255 decimal characters).
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
*/
public static final Pattern KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX =
Pattern.compile("^[^\\\\{}<>^%~#|`\\[\\]\"\\x80-\\xff]+$");
```
So basically, both "/" and "\0" are allowed whether "ozone.om.keyname.character.check.enabled" is true or not.
Maybe we still use "/", and force the that "/" is not allowed in key name, as it will cause a lot of problem, not only versioning table, also other two tables I previously mentioned.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org