Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20535 )
Change subject: WIP [CLI] Set rpc_max_message_size per available memory to accomodate huge response payloads ...................................................................... Patch Set 1: Verified+1 (20 comments) Thank you for the patch! Overall looks good to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG Commit Message: PS1: For git commit messages, please follow the 'generic git commit guidelines and good practices' that are referenced from this Kudu doc page: https://kudu.apache.org/docs/contributing.html#_submitting_patches In particular, it makes sense to keep the line length in the commit description under 72 symbols (BTW, gerrit show the ruler at the right marging that's specific for commit messages). Another good practice is keeping the subject/summary under 50 symbols, if possible (it's OK to sacrifice grammatical correctness for that -- similar to what one would see in newspapers' headlines). You could find the rationale behind in the 'generic git commit guidelines and good practices', and I'm adding a link here just in case: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines Thank you! http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@7 PS1, Line 7: accomodate nit: accommodate http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@10 PS1, Line 10: rpc nit: RPC http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@11 PS1, Line 11: add nit: adds http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@11 PS1, Line 11: rpc nit: RPC http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@12 PS1, Line 12: rpc nit: RPC http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@13 PS1, Line 13: accomodate nit: accommodate http://gerrit.cloudera.org:8080/#/c/20535/1//COMMIT_MSG@16 PS1, Line 16: atleast nit: at least http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc File src/kudu/tools/tool_main.cc: http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@255 PS1, Line 255: or nit: and http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@255 PS1, Line 255: d nit: add a period in the end of the sentence http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@256 PS1, Line 256: s nit: add a period in the end of the sentence http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@256 PS1, Line 256: Return value nit: The return value ? http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@260 PS1, Line 260: kudu::process_memory::CurrentConsumption()) For simplicity, consider dropping this part: I guess it's not going to be different from 0 if called as of now in the main() function of the CLI tools. http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@274 PS1, Line 274: rpc nit: RPC http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@275 PS1, Line 275: a rpc nit: an RPC ? http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@275 PS1, Line 275: accomodate accommodate http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@277 PS1, Line 277: int64_t nit: consider adding 'const' to express that this is an invariant in this scope http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@283 PS1, Line 283: atleast nit: at least ? http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@284 PS1, Line 284: rpc nit: RPC http://gerrit.cloudera.org:8080/#/c/20535/1/src/kudu/tools/tool_main.cc@286 PS1, Line 286: int64_t nit: consider adding 'const' to express that this is an invariant in this scope -- To view, visit http://gerrit.cloudera.org:8080/20535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic27b494bc1fde46c2a095c7291fc840a98429068 Gerrit-Change-Number: 20535 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 04 Oct 2023 18:30:33 +0000 Gerrit-HasComments: Yes
