Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17902 )
Change subject: [master] update on max_returned_locations semantics ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/17902/1/src/kudu/master/catalog_manager-test.cc File src/kudu/master/catalog_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17902/1/src/kudu/master/catalog_manager-test.cc@222 PS1, Line 222: req.clear_max_returned_locations(); // the default is 10 in protobuf > I'm curious, are we expecting to actually clear this value for our clients? The primary incentive for this change was getting rid of the hard-coded values in test scenarios like one in table_locations-itest.cc (it's a part of this changelist). The idea is that hard-coding the expected limit on the number of returned locations for the 'max_returned_locations' field might hide a bug if there are more tablets actually created. Also, it looked a bit strange to try getting the field's value for those checks in catalog_manager.cc without first verifying the field is set at all. As for the code in currently existing Kudu clients, the code is always using some heuristics to fetch up to some limit of locations: https://github.com/apache/kudu/blob/7b0a94d72de4475c6e6c3d7d78569403407cb278/src/kudu/client/meta_cache.h#L72-L77 I updated the commit description to be more clear what was the incentive behind this patch. -- To view, visit http://gerrit.cloudera.org:8080/17902 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iebc2b2295576c0b1b637fdeb95d4f9ff7b4d51c7 Gerrit-Change-Number: 17902 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 05 Oct 2021 17:55:59 +0000 Gerrit-HasComments: Yes
