Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/16204 )
Change subject: IMPALA-8125: Add query option to limit number of hdfs writer instances ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/16204/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16204/1//COMMIT_MSG@9 PS1, Line 9: This patch adds a new query option MAX_FS_WRITERS that limits the > Maybe we should call it FS instead of HDFS? Just cause the name is a bit an Done http://gerrit.cloudera.org:8080/#/c/16204/1//COMMIT_MSG@26 PS1, Line 26: - Added e2e tests to confirm that the scheduler is enforcing the limit > It seemed based on first glance that we should probably have end-to-end tes My initial approach was to cover all plan changes in the planner tests and only cover sanity checks for the scheduler changes in the e2e test. Current e2e test only covers the plan shape with multiple non leaf root nodes (RANDOM or HASH_PARTITIONED), so i think we can easily add more sanity checks for the other two shapes (UNPARTITIONED root fragment and root fragment containing scan) too. http://gerrit.cloudera.org:8080/#/c/16204/1/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/16204/1/be/src/scheduling/scheduler.cc@496 PS1, Line 496: // This implementation creates the desired number of instances while balancing them > Can you also comment what it's trying to achieve (i.e. Create the desired n Done http://gerrit.cloudera.org:8080/#/c/16204/1/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/16204/1/be/src/service/query-options.cc@685 PS1, Line 685: } > Uhh... oops. It would be good to file a separate JIRA for the missing break filed IMPALA-9966 and reverted the added breaks http://gerrit.cloudera.org:8080/#/c/16204/1/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/16204/1/common/thrift/ImpalaService.thrift@547 PS1, Line 547: // Sets an upper limit on the number of fs writer instances to be scheduled during insert. > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/16204/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/16204/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@325 PS1, Line 325: + " (id int) location '/'"); > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/16204/1/testdata/workloads/functional-planner/queries/PlannerTest/insert-hdfs-writer-limit.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-hdfs-writer-limit.test: http://gerrit.cloudera.org:8080/#/c/16204/1/testdata/workloads/functional-planner/queries/PlannerTest/insert-hdfs-writer-limit.test@6 PS1, Line 6: ---- DISTRIBUTEDPLAN > Maybe we should only include DISTRIBUTEDPLAN in these tests? I feel like th Done. I initially put it there to indicate where the exchange has been added in the distributed plan, but yea like you said its adding more noise than being helpful. http://gerrit.cloudera.org:8080/#/c/16204/1/tests/custom_cluster/test_mt_dop.py File tests/custom_cluster/test_mt_dop.py: http://gerrit.cloudera.org:8080/#/c/16204/1/tests/custom_cluster/test_mt_dop.py@105 PS1, Line 105: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/16204/1/tests/custom_cluster/test_mt_dop.py@117 PS1, Line 117: > We actually set unlock_mt_dop=true for the e2e tests, so I think this could Done http://gerrit.cloudera.org:8080/#/c/16204/1/tests/query_test/test_insert.py File tests/query_test/test_insert.py: http://gerrit.cloudera.org:8080/#/c/16204/1/tests/query_test/test_insert.py@354 PS1, Line 354: > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/16204/1/tests/query_test/test_insert.py@382 PS1, Line 382: > flake8: E231 missing whitespace after ',' Done -- To view, visit http://gerrit.cloudera.org:8080/16204 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I17c8e61b9a32d908eec82c83618ff9caa41078a5 Gerrit-Change-Number: 16204 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 17 Jul 2020 01:19:09 +0000 Gerrit-HasComments: Yes
