Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18330 )
Change subject: [tool] Fix outputs 'kudu pbc dump instance' when using -json/-json_pretty ...................................................................... Patch Set 19: (13 comments) I just took another quick look. I'm not sure current split of fs.proto makes a lot of sense. Are you sure the change in the instance metadata files size is due adding this new attribute, but not other fields recently added in the context of multi-tenancy? http://gerrit.cloudera.org:8080/#/c/18330/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18330/19//COMMIT_MSG@7 PS19, Line 7: Fix outputs 'kudu pbc dump instance' when using -json/-json_pretty Maybe, for clarity update this to be fix UUID format in 'kudu dump instance' output http://gerrit.cloudera.org:8080/#/c/18330/19//COMMIT_MSG@9 PS19, Line 9: The output of two commands below about the 'uuid' field are not the same Maybe, replace with UUID fields were output in different formats by the following commands: http://gerrit.cloudera.org:8080/#/c/18330/19//COMMIT_MSG@24 PS19, Line 24: '--debug/default --json/json_pretty ? http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/fs/fs_with_extend.proto File src/kudu/fs/fs_with_extend.proto: http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/fs/fs_with_extend.proto@22 PS19, Line 22: LBM 10 times larger than before or so (7200 bytes vs 836 bytes). Are you sure that's because of the new attribute, not because of the new 'tenant' field that has been added recently? http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/easy_json.h File src/kudu/util/easy_json.h: http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/easy_json.h@27 PS19, Line 27: pretty json or json I'm not sure this comment adds any useful information. Consider updating this: the idea is to http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/easy_json.h@72 PS19, Line 72: Pass external Json object. ? What are the ownership rules for the pointer passed? Who owns the memory pointed by the 'value' parameter? http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h@422 PS19, Line 422: bytes fields byte fields http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h@423 PS19, Line 423: json JSON http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h@423 PS19, Line 423: protobuf container What's "protobuf container" in this context? http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h@428 PS19, Line 428: plain format What's plain format in this context? I'm not sure I understand. http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.h@430 PS19, Line 430: protobuf container What's 'protobuf container'? http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.proto File src/kudu/util/pb_util.proto: http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.proto@46 PS19, Line 46: // This is for 'pbc dump' when using 'json/json-pretty' format, some bytes fields : // will output its base64-encoded text, set this option, we can output base64-decoded : // text the same as using plain format. : // This option is only used for bytes fields, if some fields with other types set this option, : // The command 'pbc dump' will ignore it. Consider rewriting this -- it reads a bit convoluted as of now. http://gerrit.cloudera.org:8080/#/c/18330/19/src/kudu/util/pb_util.proto@51 PS19, Line 51: PBC_ It doesn't have to be just the 'kudu pbc dump' tool -- that's about a way to represent byte PB fields in various human and machine-readable output. So, drop the 'PBC_' prefix. Instead, I'd suggest to name this as RAW or NO_ENCODING as opposed to any encoding, such as base64 or alike when outputting byte fields. -- To view, visit http://gerrit.cloudera.org:8080/18330 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia21afb03d9b7b4b2d4ea5aaa642701451282bebf Gerrit-Change-Number: 18330 Gerrit-PatchSet: 19 Gerrit-Owner: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Wed, 19 Jul 2023 02:28:07 +0000 Gerrit-HasComments: Yes
