Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21622 )
Change subject: KUDU-3595: Add interface to set rpc_max_message_size for C++ client ...................................................................... Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/21622/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21622/5//COMMIT_MSG@22 PS5, Line 22: apprpriate appropriate http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/client/client.h@309 PS5, Line 309: payload nit: it's not exactly payload -- that total size along with metadata, etc. http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/client/client.h@312 PS5, Line 312: /// default(FLAGS_rpc_max_message_size). nit: add a space before the parenthesis? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/client/client.h@315 PS5, Line 315: value to set Is there any special semantics with negative values provided for the parameter? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/connection.cc@525 PS5, Line 525: int64_t rpc_max_size = reactor_thread_->reactor()->messenger()->rpc_max_message_size(); nit: consider adding 'const'? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/connection.cc@530 PS5, Line 530: rpc_max_size Instead of passing this as a parameter for the constructor and storing it in the field of InboundTransfer, why not to pass just an additional parameter to the InboundTransfer::ReceiveBuffer() method? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h@114 PS5, Line 114: payload nit: it's not payload, but total size of the RPC message http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h@577 PS5, Line 577: // Maximum RPC message size set by MessengerBuilder nit: please add information on the unit (bytes?) into the comment http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h@578 PS5, Line 578: int64_t rpc_max_message_size_; An extra nit for here and elsewhere in the modified code: if staring an in-line comment with a capital letter, consider adding a period in the end -- at least that's so for the rest of the comments in this file. http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/rpc-test.cc@914 PS5, Line 914: mb.set_min_negotiation_threads(1) : .set_max_negotiation_threads(1) nit: is this crucial to have these two properties set? If not, consider not setting them http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.h@103 PS5, Line 103: int64_t rpc_max_message_size_; Should this be a constant field? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.cc@136 PS5, Line 136: nit: remove extra space? http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.cc@136 PS5, Line 136: PC max message size flag value is $2 Why is it important to report on the flag value here when the effective maximum allowed RPC size is defined by a parameter in the constructor? -- To view, visit http://gerrit.cloudera.org:8080/21622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib8feb5ba92ea604442a643e3286944564e655fa6 Gerrit-Change-Number: 21622 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Mon, 14 Oct 2024 23:23:00 +0000 Gerrit-HasComments: Yes
