Alexey Serbin 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: (5 comments) http://gerrit.cloudera.org:8080/#/c/21622/2//COMMIT_MSG Commit Message: 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 know this works? I suspect the code in PS1 isn't working as it expected, at least because the logic relies on the values of uninitialized variables. 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 are passed by value 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? 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 functionality. Once this methods is called for one instance of a KuduClient, it changes this for all the rest of the client created in the same application runtime. Also, if a Kudu client would be integrated into kudu-tserver (say, to convert REST requests into RPCs), that would change the setting of the whole application. That's not expected and it contradicts the documentation of the newly added KuduClientBuilder::rpc_max_message_size() method, which says the default is 50MiBytes. 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, similar to the 'num_reactors_' field. -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Mon, 29 Jul 2024 18:39:19 +0000 Gerrit-HasComments: Yes
