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 14: (9 comments) http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@13 PS14, Line 13: base64 encoded base64-encoded http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@15 PS14, Line 15: apis API http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@15 PS14, Line 15: base64 encoded base64-encoded http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h File src/kudu/util/pb_util.h: http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h@422 PS14, Line 422: enum class DumpFlag { Should the name of this enum contain some reference on the context where it's used, i.e. something related to UUID, e.g. UUIDTextDump? It would be great to have a comment to describe this enumeration: the context where can be used in general and where it's used at least now when its have just been introduced. Also, please document every enumeration tag as well. http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h@423 PS14, Line 423: DEFAULT That's not the best name for the enum tag. Maybe, name this as BASE64_ENCODED? http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h@424 PS14, Line 424: DECODE_UUID Maybe, name this as UUID_HEX? http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@989 PS14, Line 989: const std::string kUuidField = "uuid"; Instead of having this field name hard-coded here, consider something similar to the KUDU.redact option: that way a special custom protobuf option could be added to any protobuf field. http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@1027 PS14, Line 1027: JsonFormat json_format = JsonFormat::JSON; nit: is it possible to move this to line 1024, to be after Format::JSON but before Format::JSON_PRETTY? http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@1044 PS14, Line 1044: CHECK_OK Here and below: why not to use RETURN_NOT_OK() here? I.e., why to crash if the input isn't what it expected to be? -- 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: 14 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Mon, 12 Jun 2023 18:25:27 +0000 Gerrit-HasComments: Yes
