David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9783 )
Change subject: KUDU-16 pt 1: add server-side limits on scanners ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/scan_spec.h File src/kudu/common/scan_spec.h: http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/scan_spec.h@45 PS2, Line 45: has_limit_(false), do you really need this. can't you get away with just having a has_limit() function that tests if the limit is != 0 http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/scan_spec.cc File src/kudu/common/scan_spec.cc: http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/scan_spec.cc@72 PS2, Line 72: has_limit_ && limit_ == 0 right, same point. this seems redudant. could just call the has_limit() function http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/common/wire_protocol.h@157 PS2, Line 157: const boost::optional<int64_t>& max_num_rows_to_return = boost::none likely best move this before the pad_ argument. it's possible we remove it in the future but still keep this one. http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/tserver/scanners.h@368 PS2, Line 368: : int64_t num_rows_returned_; docs http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/tserver/tserver.proto@229 PS2, Line 229: repeated ColumnRangePredicatePB DEPRECATED_range_predicates = 3 [deprecated = true]; seems like an unrelated change that could go in a separate change set? http://gerrit.cloudera.org:8080/#/c/9783/2/src/kudu/tserver/tserver.proto@282 PS2, Line 282: // The maximum number of rows to scan. : // The scanner will automatically stop yielding results and close : // itself after reaching this number of result rows. : optional int64 limit = 15; how does this relate to 'batch_size_bytes' in the scan request below. we should at least doc it. also, did we have a limit before that was server side? if yes might be worth pondering moving it here (and making it default), since it's easier to find. -- To view, visit http://gerrit.cloudera.org:8080/9783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ca5ddff09f4910d12f80259bd11e4027f99aa7c Gerrit-Change-Number: 9783 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 23 Mar 2018 20:01:43 +0000 Gerrit-HasComments: Yes
