Hi Tsz-Wo Thanks for your reply, > BucketInfo#StorageTypeProto can be replaced with .....
I think compatibility is fine, but if we change an existing protobuf field, `proto-backwards-compatibility` will report an error unless `-DallowBreakingChanges=true` is added during compilation. But how should we handle this situation in GitHub's PR? This may cause GitHub CI compilation to fail. Xi Chen On 2024/08/20 17:52:57 Tsz Wo Sze wrote: > > ... 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 > > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@ozone.apache.org For additional commands, e-mail: dev-h...@ozone.apache.org
