Ashwani Raina 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 6: (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: appropriat > appropriate Done 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: message > nit: it's not exactly payload -- that total size along with metadata, etc. Changed to "message". 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? Done 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 parame Not that I am aware of. Even a value below 1MB would be deemed invalid per flag validator. The value may go below 1MB for test purposes though but still remain positive. 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: const int64_t rpc_max_size = reactor_thread_->reactor()->messenger()->rpc_max_message_size(); > nit: consider adding 'const'? Done 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 i This way it is a one time thing during initialisation instead of passing argument every time ReceiveBuffer is called. I prefer this way. 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: for sen > nit: it's not payload, but total size of the RPC message Done http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h@577 PS5, Line 577: int64_t rpc_max_message_size_; > nit: please add information on the unit (bytes?) into the comment Done http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/messenger.h@578 PS5, Line 578: > An extra nit for here and elsewhere in the modified code: if staring an in- Done 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_rpc_max_message_size(server_rpc_max_size) : .set_metric_entity(metric_entit > nit: is this crucial to have these two properties set? If not, consider no Done 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: const int64_t rpc_max_message_ > Should this be a constant field? Done 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? Done http://gerrit.cloudera.org:8080/#/c/21622/5/src/kudu/rpc/transfer.cc@136 PS5, Line 136: C max message size flag value is $2" > Why is it important to report on the flag value here when the effective max This is for debug purpose only. -- 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: 6 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: Thu, 17 Oct 2024 12:17:47 +0000 Gerrit-HasComments: Yes
