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: [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]
