Andrew Wong 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 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol-test.cc@323 PS9, Line 323: boost::none /* max_num_rows_to_return */, true /* pad timestamps */); > if you write this as: /* max_num_rows_to_return= */boost::none then clang-t Done http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol.cc@762 PS9, Line 762: max_num_rows_to_return > we can get rid of some branching here by doing this once up front: Done http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol.cc@771 PS9, Line 771: (*max_num_rows_to_return - rows_returned) > can also avoid this by updating a 'max_rows' above incrementally Done http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/common/wire_protocol.cc@838 PS9, Line 838: num_rows > what's the purpose of allowing this to be negative in this function? can't Done http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/tserver/tablet_server-test.cc@2072 PS9, Line 2072: lmits > typo Done http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/tserver/tablet_service.cc@485 PS9, Line 485: optional > maybe this can be non-optional and simplify code paths by just setting it t Nice, I like it. http://gerrit.cloudera.org:8080/#/c/9783/9/src/kudu/tserver/tablet_service.cc@502 PS9, Line 502: num_rows_returned_ > do we still need this member if it's redundant with the one inside the scan It's necessary for the scanned rows metrics. AFAICT, scanners (and their internal count) can batches multiple scans, whereas the metrics get updated after each HandleRowBlock() (per each batch's result collector). I'm not sure there's elegant way to refactor this out. -- 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: 9 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 30 Mar 2018 07:52:49 +0000 Gerrit-HasComments: Yes
