Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12097 )
Change subject: IMPALA-7980: Fix spinning threads because of buggy handling of num_unqueued_files_. ...................................................................... Patch Set 1: (5 comments) No major concerns, just some suggestions to reduce entropy in this code. http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node-base.h@450 PS1, Line 450: /// Number of files that have not been issued from the scanners. Comment is pretty cryptic, maybe improve it while we're looking at this logic? http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node.cc@409 PS1, Line 409: if (progress_.done()) { You could probably also add a DCHECK in Close(), something like DCHECK(!progress_.done() || num_unqueued_files_.Load() == 0) << num_unqueued_files_.Load()); http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node.cc@411 PS1, Line 411: DCHECK DCHECK_EQ (so it prints the values on failure) http://gerrit.cloudera.org:8080/#/c/12097/1/testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test File testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test: http://gerrit.cloudera.org:8080/#/c/12097/1/testdata/workloads/functional-query/queries/QueryTest/runtime_filters_wait.test@22 PS1, Line 22: select count(*) from dimtbl a join dimtbl b on a.zip=b.zip where a.name="DoesNotExist" and b.name="DoesNotExist2"; long line http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py File tests/query_test/test_runtime_filters.py: http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py@53 PS1, Line 53: """Test that a query that has global filters does not wait for them if run in LOCAL The test case you added doesn't really seem to fit this description, right? Maybe the comment just needs an update. -- 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: 1 Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 17 Dec 2018 18:32:23 +0000 Gerrit-HasComments: Yes
