Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/21622 )
Change subject: [client] KUDU-3595: Add a way to set Kudu client's rpc_max_message_size ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG@9 PS2, Line 9: clien't > nit: client's Done http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG@14 PS2, Line 14: Will try to add one either : in this patch > Definitely, it's better to add a test in this patch. Otherwise, how do you I have tested this patch using repro steps mentioned IMPALA-13202 in a kudu + impala co-dev setup by setting message max size in KuduClientBuilder object which in turn is used in client library for usage during receive. However, patch doesn't take into consideration the points you made here: https://gerrit.cloudera.org/c/21622/2/src/kudu/client/client.cc#398 I will plan to add unit test as stated above with refined approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.h@317 PS2, Line 317: const > 'const' for parameters of integer types doesn't make much sense since they Done http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc@397 PS2, Line 397: rpc_max_message_size_ > I don't think this field was initialized to 0, wasn't it? Done http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client.cc@398 PS2, Line 398: FLAGS_rpc_max_message_size = data_->rpc_max_message_size_; > I don't think this is the right way of implementing the required functional I didn't consider the possibility of client integrated into kudu-tserver. Thanks for pointing that out! I thought about the first concern from the usage perspective. My understanding was that it is likely that throughout the application runtime the value of the flag is required to be of similar size to accommodate the payload. However, that may not be the case as it takes away the flexibility of granular parameter settings for each client instance based on their individual requirements. For now, I will move this patch to WIP state to accommodate the better approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h File src/kudu/client/client_builder-internal.h: http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h@44 PS2, Line 44: rpc_max_message_size_ > To avoid using special values, consider using std::optional for this, simil Will revisit this in different approach. http://gerrit.cloudera.org:8080/#/c/21622/2/src/kudu/client/client_builder-internal.h@44 PS2, Line 44: int64_t > Would this make more sense as an unsigned value? Will revisit this in different approach. -- 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: 2 Gerrit-Owner: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jul 2024 10:16:16 +0000 Gerrit-HasComments: Yes
