-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-11351. Unify protobuf definition of StorageType #7109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d553e09
cff5792
3b1d813
016f57d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -307,6 +307,14 @@ enum ReplicationFactor { | |
| ZERO = 0; // Invalid Factor | ||
| } | ||
|
|
||
| enum StorageType { | ||
| DISK_TYPE = 1; | ||
| SSD_TYPE = 2; | ||
| ARCHIVE_TYPE = 3; | ||
| RAM_DISK_TYPE = 4; | ||
| PROVIDED_TYPE = 5; | ||
|
Comment on lines
+311
to
+315
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't recall.
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. |
||
| } | ||
|
|
||
| message ECReplicationConfig { | ||
| required int32 data = 1; | ||
| required int32 parity = 2; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,19 +178,21 @@ message StorageReportProto { | |
| optional uint64 capacity = 3 [default = 0]; | ||
| 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]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to be safe to just replace We should test it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your suggestion, I think this is compatibility, but the maven plugin And the plugin
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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] |
||
| optional bool failed = 7 [default = false]; | ||
| optional uint64 committed = 8 [default = 0]; | ||
| optional uint64 freeSpaceToSpare = 9 [default = 0]; | ||
| optional StorageType storageTypeProto = 10 [default = DISK_TYPE]; | ||
| } | ||
|
|
||
| message MetadataStorageReportProto { | ||
| required string storageLocation = 1; | ||
| optional StorageTypeProto storageType = 2 [default = DISK]; | ||
| optional StorageTypeProto storageType = 2 [default = DISK, deprecated = true]; | ||
| optional uint64 capacity = 3 [default = 0]; | ||
| optional uint64 scmUsed = 4 [default = 0]; | ||
| optional uint64 remaining = 5 [default = 0]; | ||
| optional bool failed = 6 [default = false]; | ||
| optional StorageType storageTypeProto = 7 [default = DISK_TYPE]; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| /** | ||
| * Ozone specific storage types. | ||
| */ | ||
| @Deprecated | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it really deprecated? If so, what replaces it? |
||
| public enum StorageType { | ||
| RAM_DISK, | ||
| SSD, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -754,7 +754,7 @@ message BucketInfo { | |
| required string bucketName = 2; | ||
| repeated OzoneAclInfo acls = 3; | ||
| required bool isVersionEnabled = 4 [default = false]; | ||
| required StorageTypeProto storageType = 5 [default = DISK]; | ||
| optional StorageTypeProto storageType = 5 [default = DISK, deprecated = true]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this change from required to optional backward compatible? |
||
| optional uint64 creationTime = 6; | ||
| repeated hadoop.hdds.KeyValue metadata = 7; | ||
| optional BucketEncryptionInfoProto beinfo = 8; | ||
|
|
@@ -843,7 +843,7 @@ message BucketArgs { | |
| required string volumeName = 1; | ||
| required string bucketName = 2; | ||
| optional bool isVersionEnabled = 5; | ||
| optional StorageTypeProto storageType = 6; | ||
| optional StorageTypeProto storageType = 6 [deprecated = true]; | ||
| repeated hadoop.hdds.KeyValue metadata = 7; | ||
| optional uint64 quotaInBytes = 8; | ||
| optional uint64 quotaInNamespace = 9; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusing with
org.apache.hadoop.hdds.protocol.StorageType, let's useStorageTypeProto?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both
hdds.protoandScmServerDatanodeHeartbeatProtocol.protoarepackage hadoop.hdds;so we cannot name it toStorageTypeProtoUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since they are in the same package, we could safely move the
StorageTypeProtofrom ScmServerDatanodeHeartbeatProtocol.proto to hdds.proto. The only difference is thejava_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.patchBTW, let's rename ScmServerDatanodeHeartbeatProtocol.proto to
StorageContainerDatanodeProtocol.proto, i.e. make it consistent with thejava_outer_classname.