Yuqi Du 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 16: (13 comments) Thanks for your advices and I fixed them. You can review it again. http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@12 PS14, Line 12: > It is better to describe the difference of the first command and the second Done http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@13 PS14, Line 13: > base64-encoded Done http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@14 PS14, Line 14: . kudu pbc dump instance, which is a plain output format. > Here you need to compare '--debug/default' mode with '-json/-json_pretty' m I guess you mean I should paste the two commands output at this. So I do this. http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@15 PS14, Line 15: > base64-encoded Done http://gerrit.cloudera.org:8080/#/c/18330/14//COMMIT_MSG@15 PS14, Line 15: > API Done 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: // 'BytesTextDumpFlag > Should the name of this enum contain some reference on the context where it Done http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h@423 PS14, Line 423: when u > That's not the best name for the enum tag. Maybe, name this as BASE64_ENCO ok http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.h@427 PS14, Line 427: enum class BytesFieldDumpFlag { > Use DECODE_UUID as the default value is a little confusing. There is a Dump Yes. I fixed the names. 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 simil I should study about this http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@990 PS14, Line 990: decode it from base6 > decode it from base64 format Done http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@994 PS14, Line 994: // Maybe it's necessary to consider other 'bytes' fields which can be dumped. > If the whole message is encoded in base64 format, can we decode whole messa No. Only bytes fields, in 'kudu pbc dump instance' is the uuid. Other field is normal , not based64-encoded. 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 It's hard to make the two 'case' using a common variable except moving this statement before switch. L1029 has an example. So, I think this statement at here it ok. http://gerrit.cloudera.org:8080/#/c/18330/14/src/kudu/util/pb_util.cc@1044 PS14, Line 1044: RETURN_N > Here and below: why not to use RETURN_NOT_OK() here? I.e., why to crash if Yes. This CHECK is not necessary. I fix them. -- 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: 16 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: Tue, 13 Jun 2023 16:37:06 +0000 Gerrit-HasComments: Yes
