> ... because the proto message `BucketInfo#StorageTypeProto` is marked as `required`. ...
BucketInfo#StorageTypeProto can be replaced with the new enum since "enum is compatible with int32, uint32, int64, and uint64 in terms of wire format". The new StorageTypeProto in hdds.proto will be compatible with the existing StorageTypeProto in OmClientProtocol.proto. > Indeed, proto2 does not support adding `[deprecated = true]` to enums. ... The recommendation is from proto2 guide https://protobuf.dev/programming-guides/proto2/#enum . Interestingly, proto2 (2.6.1) does support `[deprecated = true]`. Unfortunately, we are using 2.5.0. Just have tried below: diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index 32bba26608..1ea2a0fe05 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -747,6 +747,8 @@ enum StorageTypeProto { SSD = 2; ARCHIVE = 3; RAM_DISK = 4; + NEW_TYPE_1 = 8; + NEW_TYPE_2 = 9 [deprecated=true]; } enum BucketLayoutProto { diff --git a/pom.xml b/pom.xml index b5a6323bed..431ad17d28 100644 --- a/pom.xml +++ b/pom.xml @@ -191,7 +191,7 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs <!-- ProtocolBuffer version, used to verify the protoc version and --> <!-- define the protobuf JAR version --> - <proto2.hadooprpc.protobuf.version>2.5.0</proto2.hadooprpc.protobuf.version> + <proto2.hadooprpc.protobuf.version>2.6.1</proto2.hadooprpc.protobuf.version> <proto3.hadooprpc.protobuf.version>3.7.1</proto3.hadooprpc.protobuf.version> <hadoop-thirdparty.version>1.1.1</hadoop-thirdparty.version> Tsz-Wo On Tue, Aug 20, 2024 at 10:24 AM Xi Chen <che...@apache.org> wrote: > Hi Tsz-Wo, Abhishek Pal > > Indeed, proto2 does not support adding `[deprecated = true]` to enums. > Therefore, I intend to use the `restrict-imports-enforcer-rule` plugin to > limit the usage of these enums that are effectively deprecated, by > prohibiting their import. > > However, this approach cannot entirely prevent the usage of deprecated > enums because the proto message `BucketInfo#StorageTypeProto` is marked as > `required`. While we can change it to `optional`, we still need to assign > it a value to maintain API interface compatibility. > > Then I will define the new `StorageType` in `hdds.proto` and let the > project use the newly defined `StorageType` as much as possible. > > Xi Chen > > > On 2024/08/19 18:49:56 Abhishek Pal wrote: > > > According to https://protobuf.dev/programming-guides/proto2/#enum , we > > should add [deprecated=true] to each field instead of removing the old > > protos. Please try. > > > > It seems in the current proto2 version we are using deprecated fields are > > not being supported in enums. > > Also the reserved keyword that is used to reserve field IDs is not > > supported either in proto2. > > This was found while trying to remove the listTrash and recoverTrash APIs > > as a part of HDDS-11251 > > > > On Mon, 19 Aug 2024 at 22:05, Tsz Wo Sze <szets...@gmail.com> wrote: > > > > > > Will there be any compatibility issues after moving enum > > > `StorageTypeProto` from `ScmServerDatanodeHeartbeatProtocol.proto` to > > > `hdds.proto` and standardize, and change all the places where > > > `StorageTypeProto` is used in the existing protobuf to > > > `hdds.proto#StorageTypeProto`? > > > > > > There are different levels of compatibility: > > > > > > For binary compatibility, it is okay since > > > "enum is compatible with int32, uint32, int64, and uint64 in terms of > wire > > > format" > > > according to https://protobuf.dev/programming-guides/proto2/#updating > > > It is easy to test: write both the new and the old protos to files and > then > > > compare the binaries. > > > > > > For API compatibility, the Java classes will be different. The > question: > > > are we using it in a public API? It also seems not since we are using > > > - org.apache.hadoop.hdds.protocol.StorageType > > > > > > in the public APIs. (e.g. OzoneBucket.getStorageType(). ) > > > > > > According to https://protobuf.dev/programming-guides/proto2/#enum , we > > > should add [deprecated=true] to each field instead of removing the old > > > protos. Please try. > > > > > > BTW, we also have another StorageTypeProto defined > > > in OmClientProtocol.proto. We should deprecate it as well. > > > > > > Please file a JIRA for the further discussion. > > > > > > Tsz-Wo > > > > > > > > > > > > > > > On Mon, Aug 19, 2024 at 2:17 AM mrchenx <mrch...@126.com> wrote: > > > > > > > Dear Ozone Devs, > > > > > > > > > > > > > > > > > > > > I am currently developing some features related to StoragePolicy and > have > > > > encountered an issue for which I am seeking a solution. > > > > > > > > > > > > > > > > > > > > ## What I want to do > > > > > > > > - Move the enum `StorageTypeProto` from > > > > `ScmServerDatanodeHeartbeatProtocol.proto` to `hdds.proto`. > > > > > > > > - Most of the protobuf definitions that need to be imported in our > > > > project are located in `hdds.proto`, so I want to move enum > > > > `StorageTypeProto` to `hdds.proto`. > > > > > > > > > > > > > > > > > > > > - Standardize the usage of `hdds.proto#StorageTypeProto` across all > > > > protobuf files in the project. > > > > > > > > - This would require removing > > > > `ScmServerDatanodeHeartbeatProtocol.proto#StorageTypeProto` and > > > > `OmClientProtocol.proto#StorageTypeProto`. > > > > > > > > > > > > > > > > > > > > ## The error I encountered > > > > > > > > However, I encountered an issue where `proto-backwards-compatibility` > > > > throws errors after moving the `enum StorageTypeProto`. > > > > > > > > > > > > > > > > > > > > ```bash > > > > > > > > [INFO] --- > > > > proto-backwards-compatibility:1.0.7:backwards-compatibility-check > > > (default) > > > > @ hdds-interface-server --- > > > > > > > > //... > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" field: "ARCHIVE" has been > removed, > > > > but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" field: "DISK" has been removed, > but > > > > is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" field: "PROVIDED" has been > removed, > > > > but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" field: "RAM_DISK" has been > removed, > > > > but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" field: "SSD" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" integer: "1" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" integer: "2" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" integer: "3" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" integer: "4" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > [ERROR] CONFLICT: "StorageTypeProto" integer: "5" has been removed, > but > > > is > > > > not reserved [ScmServerDatanodeHeartbeatProtocol.proto] > > > > > > > > ``` > > > > > > > > > > > > > > > > > > > > ## My question > > > > > > > > - Will there be any compatibility issues after moving enum > > > > `StorageTypeProto` from `ScmServerDatanodeHeartbeatProtocol.proto` to > > > > `hdds.proto` and standardize, and change all the places where > > > > `StorageTypeProto` is used in the existing protobuf to > > > > `hdds.proto#StorageTypeProto`? > > > > > > > > - According to my tests, when I add the compile flag > > > > `-DallowBreakingChanges=true` to bypass the errors, old clients can > still > > > > successfully execute create/list bucket operations. > > > > > > > > - As I understand it, the `field id` of a protobuf enum is what > > > matters, > > > > as the `id` is the valid payload, while changes to the enum's field > names > > > > or the package in which the enum is defined should not cause > > > compatibility > > > > issues (though we will need to update the relevant Java code to > reflect > > > > these changes). > > > > > > > > > > > > > > > > > > > > - If this does not cause compatibility issues, how should I handle > the > > > > errors reported by `proto-backwards-compatibility`? > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@ozone.apache.org > For additional commands, e-mail: dev-h...@ozone.apache.org > >
