Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9757 )
Change subject: IMPALA-6679,IMPALA-6678: reduce scan reservation ...................................................................... Patch Set 10: Code-Review+2 (4 comments) It'd be good to get Bikram's final review as well. Also please see if Alex wants to take a look at the fe changes. http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node-base.cc@538 PS10, Line 538: } I think you forgot to refactor this loop? or decided it wasn't worth it? http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.cc@228 PS10, Line 228: buffer_pool_client_.GetReservation() curr_reservation http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.cc@276 PS10, Line 276: // The node's min reservation is for the first thread so we don't need to check nit: maybe move this comment down a line http://gerrit.cloudera.org:8080/#/c/9757/8/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: http://gerrit.cloudera.org:8080/#/c/9757/8/be/src/exec/scanner-context.cc@78 PS8, Line 78: void ScannerContext::ReleaseCompletedResources(bool done) { > I documented this in the comment of this method -seemed like the place it w yeah, the question just came up while i was reading this code -- agree the header file is the right place. -- To view, visit http://gerrit.cloudera.org:8080/9757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc80e05118a9eef72cac8e2308418122e3ee0842 Gerrit-Change-Number: 9757 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 25 Apr 2018 19:47:33 +0000 Gerrit-HasComments: Yes
