Todd Lipcon 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-tidy will check that the param name matches 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: int max_rows = max_num_rows_to_return.value_or(block.nrows()); (this is a very hot function on the scan path) 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 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 it just be checked at a higher level and be a DCHECK here to simplify some stuff a bit? 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 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 to row_block.nrows() here and get rid of the optional processing deeper down? 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 scannner? -- 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 01:53:48 +0000 Gerrit-HasComments: Yes
