Bikramjeet Vig 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 (9 comments) Just a few nits. Carrying over Dan's +2 http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-parquet-scanner.cc@1519 PS10, Line 1519: min_buffer_size, max_buffer_size nit: if this is always going to be the min/max buffer_size taken from io_mgr, we can just remove the need for these input params http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-parquet-scanner.cc@1526 PS10, Line 1526: int64_t HdfsParquetScanner::ComputeIdealReservation( : const vector<int64_t>& col_range_lengths) { : DiskIoMgr* io_mgr = ExecEnv::GetInstance()->disk_io_mgr(); : int64_t ideal_reservation = 0; : for (int64_t len : col_range_lengths) { : ideal_reservation += io_mgr->ComputeIdealBufferReservation(len); : } : return ideal_reservation; : } nit: now that we have factored out the logic to io_mgr and made this methods really small, perhaps we can move this into DivideReservationBetweenColumns() http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.h@67 PS10, Line 67: IncreaseReservationForExtraThread() outdated comment http://gerrit.cloudera.org:8080/#/c/9757/10/be/src/exec/hdfs-scan-node.h@67 PS10, Line 67: scanner thread is responsible : /// for staying within the reservation handed off to it. mention that the scanner thread can scale up reservation in case of columnar formats like parquet. http://gerrit.cloudera.org:8080/#/c/9757/6/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/9757/6/be/src/exec/hdfs-scan-node.cc@226 PS6, Line 226: possible. Must ho > We're actually holding the lock everywhere that we touch spare_reservation_ Oh I see, did not realize spare_reservations was protected by the lock http://gerrit.cloudera.org:8080/#/c/9757/6/be/src/exec/hdfs-scan-node.cc@481 PS6, Line 481: : status = scanner->ProcessSplit(); > There are multiple streams per reservation so it's not generally true that Got it. Thanks for the explanation http://gerrit.cloudera.org:8080/#/c/9757/6/be/src/exec/hdfs-scan-node.cc@518 PS6, Line 518: > I don't think this is the right place to document global assumptions about Agreed http://gerrit.cloudera.org:8080/#/c/9757/6/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/9757/6/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@154 PS6, Line 154: // TODO: is it worth making this a tunable query option? > I think it would just be a safety valve if the planner made a bad decision. Good point, might be a good idea to have this from the beginning http://gerrit.cloudera.org:8080/#/c/9757/10/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/9757/10/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@781 PS10, Line 781: partitionBytes += fileDesc.getFileLength(); nit: can move this to the same loop at L786 if you feel it dosent impact code readability too much -- 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 22:53:16 +0000 Gerrit-HasComments: Yes
