[GitHub] [ozone] rakeshadr commented on a change in pull request #2357: HDDS-5362. [FSO] Support bucket layouts in OM

[GitBox <gi...@apache.org>]

rakeshadr commented on a change in pull request #2357:
URL: https://github.com/apache/ozone/pull/2357#discussion_r677082778



##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -528,6 +529,12 @@ enum StorageTypeProto {
     RAM_DISK = 4;
 }
 
+enum BucketTypeProto {

Review comment:
       Thanks a lot @marton for the comments.
   
   > I am against removing the Map<String,String> as it can be used not only for storing bucket types but storing any other metadata. Also, it's an incompatible change if we remove it.
   
   To make it clear about the`Map<String,String>` entity - this patch will not remove `KeyValue` attribute from the BucketInfo and it will remain the same. Any upcoming feature or behaviour can still make use of this `KeyValue` attribute and build it.
   
   _OmClientProtocol.proto_
   ```
       message BucketInfo {
       .....
       repeated hadoop.hdds.KeyValue metadata = 7;
       .....
   + optional BucketLayoutProto bucketLayout = 18;
   ```
   
   This patch is moving the bucket layout information stored in the **Map to an enum attribute.**
   
   > Also, it's an incompatible change if we remove it.
   
   IIUC, since the FSO(prefix) feature is not released in any of the Ozone version we have still the opportunity to refactor/redesign the proto part. Please let me know if I missed anything.
   
   Following are the points while evaluating `enum` over `Map<String, String>`.
   
   - enum has options to limit the set of valid values and will validate the input efficiently. Please refer, https://picocli.info/#_enum_types
   - protobuf defaults to the enum constant at ordinal 0 and not worrying about null checks in a programatic way(easily avoid NPE cases). Not only from the commandline, while reading back the old data from DB, where the bucket layout will not exists in DB.
   - according to me enum makes code more maintainable and easy to read
   
   I'm completely open for a better approach. With map, we have to do all the above in a programatic way and its doable. Please let me know your feedback. Thanks again!




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org