Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12097 )
Change subject: IMPALA-7980: Fix spinning because of buggy num_unqueued_files_. ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/12097/5/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/12097/5/be/src/exec/hdfs-scan-node-base.h@465 PS5, Line 465: remaining_scan_range_submission > I am now trying remaining_scan_range_submissions_. I'd argue that it's all Thanks for the explanation. I suppose the initial source of confusion had to do with the way to interpret this variable. In particular, there is a minor distinction between (1) the remaining scan ranges to be issued to the IO subsystem for the rest of the scan nodes' lifetime and (2) the "new work" to be picked up by scanner threads. I believe this variables refer to (2). In other words, the scanner thread which picks up the footer range may still submit one or more scan ranges for the actual split it's responsible for after this variable has gone to 0. As far as the scanner thread loop is concerned, there is no more new work so it's safe to exit. Not sure if there is a better name to explain this here but I guess some (hopefully succinct) comments may help. http://gerrit.cloudera.org:8080/#/c/12097/5/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12097/5/be/src/exec/hdfs-scan-node.cc@79 PS5, Line 79: : Status HdfsScanNode::GetNext(RuntimeState* state, RowBatch* row_batch, bool* eos) { : SCOPED_TIMER(runtime_profile_->total_time_counter()); : Sco > You were right and we can move this into Close() once the threads have been I was just wondering if the if-stmt body is empty as DCHECK is not compiled in release build, the if-check automatically becomes dead code. That said, I am not sure the compiler is always smart enough to remove the if-check as the conjuncts being evaluated may have side-effects. Anyhow, this is just a minor point. Feel free to ignore. http://gerrit.cloudera.org:8080/#/c/12097/6/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12097/6/be/src/exec/hdfs-scan-node.cc@196 PS6, Line 196: !(ranges_issued_barrier_.pending() != 0) Why not ranges_issued_barrier_.pending() == 0 ? http://gerrit.cloudera.org:8080/#/c/12097/6/be/src/exec/hdfs-scan-node.cc@197 PS6, Line 197: && initial_ranges_issued_ : && progress_.done()) { nit: one line -- To view, visit http://gerrit.cloudera.org:8080/12097 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I133de13238d3d05c510e2ff771d48979125735b1 Gerrit-Change-Number: 12097 Gerrit-PatchSet: 6 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 30 Jan 2019 00:43:42 +0000 Gerrit-HasComments: Yes
