Philip Zeyliger 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: (9 comments) Getting the DCHECK's just right is proving to be ... interesting. I'm whittling down the test failures... I'll post a new patch once I get through that, which is proving to take a while. 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 log Done http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node-base.cc@502 PS1, Line 502: SkipFile(format, file); > Does anyone else find it surprising that this method had side effects? Perh Yeah, I was surprised by that too. I changed it as you suggested. Ther's only one caller so it was straightforward. 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 Done 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) Done http://gerrit.cloudera.org:8080/#/c/12097/1/be/src/exec/hdfs-scan-node.cc@469 PS1, Line 469: // File ranges haven't been issued yet, skip entire file : SkipFile(partition->file_format(), desc); > I guess this is slightly subtle here as we never decrement "num_unqueued_fi Yep. Added a comment here. 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 Done 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? I looked deeper and the test_file_filtering captures exactly the same thing. I've removed my new test case. http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py@59 PS1, Line 59: s > flake8: E501 line too long (92 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/12097/1/tests/query_test/test_runtime_filters.py@96 PS1, Line 96: s > flake8: E501 line too long (92 > 90 characters) Done -- 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: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 21 Dec 2018 21:17:01 +0000 Gerrit-HasComments: Yes
