HDDS-11351. Unify protobuf definition of StorageType #7109

xichen01 · 2024-08-22T15:25:54Z

What changes were proposed in this pull request?

Unify protobuf definition of storageType.

Context

We have multiple storageType enum definition in different protobuf files (ScmServerDatanodeHeartbeatProtocol.proto#StorageTypeProto and OmClientProtocol.proto#StorageTypeProto).
For the future, we can more easily use the same storageType protobuf definition in multiple places. We need to remove the existing storageType protobuf and define a new storageType definition in hdds.proto.
Most of the protobuf definitions that need to be imported in our project are located in hdds.proto, so we should to define storageType enum in hdds.proto

Modifications

~~Upgrade proto2.hadooprpc.protobuf.version form 2.5.0 to 2.6.1, the proto 2.6.1 start supports the enum deprecated syntax.~~
Mark ScmServerDatanodeHeartbeatProtocol.proto#StorageTypeProto and OmClientProtocol.proto#StorageTypeProto as deprecated = true.
Add a new storageType enum definition hdds.proto#StorageType, and replace ScmServerDatanodeHeartbeatProtocol.proto#StorageTypeProto, and restrict import of ScmServerDatanodeHeartbeatProtocol (by maven plugin restrict-imports-enforcer-rule)

The OmBucketInfo#storageType will be removed in the future, because Buckets should not have a StorageType field, and should use StoragePolicy instead of StorageType

Compatibility

In StorageLocationReport, StorageType is only used in UsageMetrics. Version changes may cause this indicator to be inaccurate, but it will not affect the main business process.
ScmServerDatanodeHeartbeatProtocol#StorageTypeProto.storageType (id 2) default value is DISK, so deprecate it will not cause NPE issues

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-11351

How was this patch tested?

existing test.

xichen01 · 2024-08-22T15:29:18Z

@szetszwo, @devabhishekpal Could you help to review this?

xichen01 · 2024-08-27T08:51:38Z

There are some compatibility issues If we upgrade the protobuf version from 2.5.0 to 2.6.1, this may introduce some risks, so let we keep the protobuf version unchanged.
We can use maven plugin restrict-imports-enforcer-rule to disable the "deprecated" enum.

szetszwo

@xichen01 , thanks for working on this! Please see my suggestions inlined.

szetszwo · 2024-10-17T16:29:19Z

hadoop-hdds/interface-client/src/main/proto/hdds.proto

    ZERO = 0; // Invalid Factor
 }

+enum StorageType {


To avoid confusing with org.apache.hadoop.hdds.protocol.StorageType, let's use StorageTypeProto?

Both hdds.proto and ScmServerDatanodeHeartbeatProtocol.proto are package hadoop.hdds; so we cannot name it to StorageTypeProto

Since they are in the same package, we could safely move the StorageTypeProto from ScmServerDatanodeHeartbeatProtocol.proto to hdds.proto. The only difference is the java_outer_classname. Fortunately, ScmServerDatanodeHeartbeatProtocol.proto is a non-user facing internal protocol and the change is wire compatible. So there is no compatibility issues. See https://issues.apache.org/jira/secure/attachment/13075616/7109_review.patch

BTW, let's rename ScmServerDatanodeHeartbeatProtocol.proto to StorageContainerDatanodeProtocol.proto, i.e. make it consistent with the java_outer_classname.

szetszwo · 2024-10-17T16:31:58Z

hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto

  optional uint64 scmUsed = 4 [default = 0];
  optional uint64 remaining = 5 [default = 0];
-  optional StorageTypeProto storageType = 6 [default = DISK];
+  optional StorageTypeProto storageType = 6 [default = DISK, deprecated = true];


It seems to be safe to just replace StorageTypeProto with Hdds.StorageTypeProto.

See https://stackoverflow.com/questions/68239684/protobuf-is-it-backward-compatible-to-move-message-to-imported-file

We should test it.

Thanks for your suggestion, I think this is compatibility, but the maven plugin proto-backwards-compatibility will report error say the "field CONFLICT".

And the plugin proto-backwards-compatibility not support to ignore a specific field.

Please try https://issues.apache.org/jira/secure/attachment/13075616/7109_review.patch

This will also cause proto-backwards-compatibility to report an error. It seems that it is difficult for us to change the existing proto message

[INFO] --- proto-backwards-compatibility:1.0.7:backwards-compatibility-check (default) @ hdds-interface-server --- [INFO] protolock cmd line: /Users/xichen/community/ozone/hadoop-hdds/interface-server/target/protolock-bin/protolock status --lockdir=/Users/xichen/community/ozone/hadoop-hdds/interface-server/target/classes --protoroot=/Users/xichen/community/ozone/hadoop-hdds/interface-server/target/classes [INFO] CONFLICT: "StorageTypeProto" field: "ARCHIVE" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" field: "DISK" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" field: "PROVIDED" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" field: "RAM_DISK" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" field: "SSD" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" integer: "1" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" integer: "2" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" integer: "3" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" integer: "4" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto] [INFO] CONFLICT: "StorageTypeProto" integer: "5" has been removed, but is not reserved [ScmServerDatanodeHeartbeatProtocol.proto]

# Conflicts: # hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java # pom.xml

xichen01 · 2025-03-22T11:17:47Z

@szetszwo PTAL.

szetszwo · 2025-03-22T16:57:20Z

@xichen01 , could you take my previous review comments #7109 (review) ?

szetszwo

@xichen01 , thanks for taking a look. I have some suggestions. Please see the comments inlined.

szetszwo · 2025-03-22T18:00:03Z

hadoop-hdds/interface-client/src/main/proto/hdds.proto

    ZERO = 0; // Invalid Factor
 }

+enum StorageType {


Since they are in the same package, we could safely move the StorageTypeProto from ScmServerDatanodeHeartbeatProtocol.proto to hdds.proto. The only difference is the java_outer_classname. Fortunately, ScmServerDatanodeHeartbeatProtocol.proto is a non-user facing internal protocol and the change is wire compatible. So there is no compatibility issues. See https://issues.apache.org/jira/secure/attachment/13075616/7109_review.patch

BTW, let's rename ScmServerDatanodeHeartbeatProtocol.proto to StorageContainerDatanodeProtocol.proto, i.e. make it consistent with the java_outer_classname.

szetszwo · 2025-03-22T18:00:28Z

hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto

  optional uint64 scmUsed = 4 [default = 0];
  optional uint64 remaining = 5 [default = 0];
-  optional StorageTypeProto storageType = 6 [default = DISK];
+  optional StorageTypeProto storageType = 6 [default = DISK, deprecated = true];


Please try https://issues.apache.org/jira/secure/attachment/13075616/7109_review.patch

szetszwo · 2025-03-29T16:59:30Z

@xichen01 , Currently, we are voting 2.0.0. How about getting this in 2.0.0? Then, we can legitimately update the protolock file.

Cc @jojochuang , @adoroszlai

xichen01 · 2025-03-30T09:17:47Z

@xichen01 , Currently, we are voting 2.0.0. How about getting this in 2.0.0? Then, we can legitimately update the protolock file.

Cc @jojochuang , @adoroszlai

It looks fine to me, and if we do it, do we need to modify both proto field and protolock in one PR?

szetszwo · 2025-03-30T21:13:45Z

... both proto field and protolock in one PR?

I believe the answer is yes, although it is not mentioned in the release guideline

https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173085374#OzoneReleaseGuideline-Buildandcommittheproto.lockchange

Could you update this PR? We should get this merged soon.

jojochuang · 2025-04-01T21:30:22Z

hadoop-hdds/interface-client/src/main/proto/hdds.proto

+    DISK_TYPE = 1;
+    SSD_TYPE = 2;
+    ARCHIVE_TYPE = 3;
+    RAM_DISK_TYPE = 4;
+    PROVIDED_TYPE = 5;


I don't recall who, but back in HDFS land after storage policy feature was implemented, someone (Arpit or Anu?) regrettably said it was a mistake to have these types continuous, because it wasn't possible to add any storage types in between. It might be a good idea to space them out.

@szetszwo do you recall?

I also don't recall.

... it was impossible to insert any new storage types in between. ...

Not really -- we may use 1.1 if there is a need.

Also, why it has to insert in between? We may put the new policy at the end. I don't think Storage policy has a total ordering.

jojochuang · 2025-04-01T21:47:02Z

hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto

    repeated OzoneAclInfo acls = 3;
    required bool isVersionEnabled = 4 [default = false];
-    required StorageTypeProto storageType = 5 [default = DISK];
+    optional StorageTypeProto storageType = 5 [default = DISK, deprecated = true];


is this change from required to optional backward compatible?
Imagine a new OM server and an old client. The new server omits this field while client expects this field. Wouldn't the client fail to parse?

jojochuang · 2025-04-01T22:00:26Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/hdds/protocol/StorageType.java

 /**
 * Ozone specific storage types.
 */
+@Deprecated


is it really deprecated? If so, what replaces it?

szetszwo · 2025-04-01T22:38:18Z

@jojochuang , please review #8208 instead. We have a slightly different approach now.

@xichen01 , let's close this in order to avoid confusion?

adoroszlai · 2025-04-02T11:04:01Z

Superseded by 8208.

HDDS-11351. Unify protobuf definition of StorageType

d553e09

Recovery protobuf2 version to 2.5.0

cff5792

xichen01 requested review from adoroszlai and devmadhuu August 27, 2024 08:53

adoroszlai requested review from errose28 and szetszwo and removed request for devmadhuu October 17, 2024 15:56

szetszwo reviewed Oct 18, 2024

View reviewed changes

adoroszlai marked this pull request as draft February 15, 2025 16:29

adoroszlai and others added 2 commits February 18, 2025 17:54

Merge remote-tracking branch 'origin/master' into HDDS-11351

3b1d813

Merge branch 'master' into HDDS-11351

016f57d

# Conflicts: # hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java # pom.xml

szetszwo reviewed Mar 22, 2025

View reviewed changes

szetszwo mentioned this pull request Apr 1, 2025

HDDS-12750. Move StorageTypeProto from ScmServerDatanodeHeartbeatProtocol.proto to hdds.proto #8208

Merged

jojochuang reviewed Apr 1, 2025

View reviewed changes

adoroszlai closed this Apr 2, 2025

HDDS-11351. Unify protobuf definition of StorageType #7109

HDDS-11351. Unify protobuf definition of StorageType #7109

Uh oh!

Conversation

xichen01 commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Context

Modifications

Compatibility

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

xichen01 commented Aug 22, 2024

Uh oh!

xichen01 commented Aug 27, 2024

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xichen01 commented Mar 22, 2025

Uh oh!

szetszwo commented Mar 22, 2025

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Mar 29, 2025

Uh oh!

xichen01 commented Mar 30, 2025

Uh oh!

szetszwo commented Mar 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Apr 1, 2025

Uh oh!

adoroszlai commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xichen01 commented Aug 22, 2024 •

edited

Loading

szetszwo Mar 22, 2025 •

edited

Loading

szetszwo Mar 22, 2025 •

edited

Loading

szetszwo Apr 1, 2025 •

edited

Loading