You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2020/05/15 21:17:00 UTC
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15926
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
Testing:
- added a regression test which would almost always fail without
this patch
TODO:
- Run core tests on ASAN build
- Run exhaustive tests
- Do perf analysis to ensure there is no regression
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M tests/query_test/test_mt_dop.py
15 files changed, 613 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/1
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 7:
(1 comment)
will rebase in the next patchset
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc
File be/src/scheduling/scheduler-test.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc@725
PS1, Line 725: // Test scheduling algorithm for load-balancing scan ranges within a host.
> I would prefer to keep the test as a sanity check
Done
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 19:46:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#4).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 677 insertions(+), 461 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/4
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6151/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 19:24:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 2:
(26 comments)
http://gerrit.cloudera.org:8080/#/c/15926/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15926/1//COMMIT_MSG@39
PS1, Line 39: Testing:
> Can we add some more test coverage for mt_dop for edge cases like Avro/Parq
added some more tests as requested, however there are currently no tests with avro files that split across multiple blocks, so will have to generate those separately and add. Will look into it.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@174
PS1, Line 174: /// The following public methods are only used by MT scan nodes.
> Can we add DCHECKS to these functions to make sure that they're only used b
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@176
PS1, Line 176:
> nit: mismatched quotes
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@237
PS1, Line 237: AtomicInt32 remaining_scan_range_submissions_{0};
> nit: would usually init these as
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@240
PS1, Line 240: use_mt_scan_node
> nit: needs trailing _
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@242
PS1, Line 242: /// The following are only used by MT scan nodes.
> nit: might want to make this comment more prominent
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@246
PS1, Line 246:
> .
removed the variable.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@247
PS1, Line 247: mutex scan_range_submissio
> Not sure I understand why this is needed, since you can infer it from 'rema
Yes that was the original intention, but we can do without it as well.
Removed as suggested.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@317
PS1, Line 317: /// Per scanner type codegen'd fn.
> I'm a little skeptical about introducing mutable state into the PlanNode st
As discussed offline, we'll leave the mutable state in here for now and re-consider if more exec nodes require shared mutable state in the future.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@671
PS1, Line 671:
> nit: separate
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@766
PS1, Line 766: boost::shared_mutex bytes_read_per_col_lock_;
> Consider inlining this function - the callsites might actually be easier to
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@830
PS1, Line 830: /// debug action specified for the query.
> nit: extra space after ///. I think the comment could be fleshed out a litt
Done. Also added comments in the child classes.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@272
PS1, Line 272: null
> nit: nullptr while you're here
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@315
PS1, Line 315: Set u
> nit: Set up
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@321
PS1, Line 321: // Distribute the work evenly for issuing initial scan ranges.
> This is just to distribute the work for issuing initial scan ranges, right?
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@327
PS1, Line 327: vector<HdfsFileDesc*>* curr_file_list =
> This code feels more complex than it needs to be - it has a bunch of edge c
yea I had initially went with the round-robin implementation, but i ended up with this overkill optimization to avoid hashing the instance_id to fetch its associated object.
I'll replace it with your suggested edit. Let me know if you feel strongly about switching to round-robin and I'll change it.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@354
PS1, Line 354: if (result == path_to_materialized_slot_idx_.end()) {
> nit: nullptr
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@420
PS1, Line 420: ddBytesReadCounters();
> missed this. Need to switch this to a better way of passing the shared stat
I thought a lot about this and decided to keep this as it is.
To pass in a non-const object would mean either we make the whole ExecTree creation path non-cast or create a map in fragment state and have the instances pull them later. Neither of them seemed ideal and added unnecessary complications. So for now, will leave this as an exception and move to a more complicated approach if other exec nodes also need to have shared state in the future.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1226
PS1, Line 1226: while (scan_range_queue_.empty() && remaining_scan_range_submissions_.Load() > 0
> Doesn't this block any instances from processing scan ranges until all have
the "scan_range_queue_.empty()" would actually let it process scan ranges without waiting for the last scan range to be submitted.
Regardless, your suggested restructure is clearer so going ahead with that. I also removed got rid of last_scan_range_submitted_ like you suggested
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1234
PS1, Line 1234: }
> nit: missing space before ( here and below
removed in the restructure
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1238
PS1, Line 1238: }
> Is this check actually necessary? It's needed in the loop because otherwise
removed in the restructure
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1244
PS1, Line 1244:
> This is a little janky because we're signalling a query-level CV as part of
True. The alternative was to implement a query or fragment-state level cancellation mechanism similar to the one in RuntimeState, which sounded like an overkill unless there are other subsystems that would eventually use it.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc
File be/src/exec/hdfs-scan-node-mt.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc@140
PS1, Line 140: bool at_front = false;
> Seems weird to pass in the bool on one branch and not the other. Could make
Done
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc@142
PS1, Line 142: at_front = true;
> DCHECK that it's TAIL on the else branch
removed the extra branch
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc@73
PS1, Line 73: HdfsScanNode::HdfsScanNode(ObjectPool* pool, const HdfsScanPlanNode& pnode,
> Can we add a DCHECK or anything to make sure that there is only one instanc
not sure about the full context, but do you mean that a DCHECK(use_mt_scan_node_ || instance_ctx.size()==1) will suffice
http://gerrit.cloudera.org:8080/#/c/15926/1/tests/query_test/test_mt_dop.py
File tests/query_test/test_mt_dop.py:
http://gerrit.cloudera.org:8080/#/c/15926/1/tests/query_test/test_mt_dop.py@110
PS1, Line 110: o
> flake8: E131 continuation line unaligned for hanging indent
Done
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 May 2020 02:45:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 8: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5912/
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 30 May 2020 04:20:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 9:
One of the tests i added set the mt_dop to 2 which was being carried over to other tests. Fixed that.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 18:22:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 4:
Rebased
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 20:26:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5925/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 18:23:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
- Added missing lifecycle events from MT scan nodes.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Reviewed-on: http://gerrit.cloudera.org:8080/15926
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 702 insertions(+), 415 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 11
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@420
PS1, Line 420: shared_state_(const_cast<ScanRangeSharedState*>(&(pnode.shared_state_)))
missed this. Need to switch this to a better way of passing the shared state that does not involve a const cast.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 May 2020 21:21:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 5:
LGTM once the test is added back
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 19:34:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6083/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 May 2020 22:08:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6169/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 20:33:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 1:
(25 comments)
This is really nice!
http://gerrit.cloudera.org:8080/#/c/15926/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15926/1//COMMIT_MSG@39
PS1, Line 39: Testing:
Can we add some more test coverage for mt_dop for edge cases like Avro/Parquet files split across blocks? I think we have non-mt_dop tests for those.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@174
PS1, Line 174: /// The following public methods are only used by MT scan nodes.
Can we add DCHECKS to these functions to make sure that they're only used by the mt scan node?
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@176
PS1, Line 176:
nit: mismatched quotes
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@237
PS1, Line 237: AtomicInt32 remaining_scan_range_submissions_ = { 0 };
nit: would usually init these as
remaining_scan_range_submissions_{0}
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@240
PS1, Line 240: use_mt_scan_node
nit: needs trailing _
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@242
PS1, Line 242: /// The following are only used by MT scan nodes.
nit: might want to make this comment more prominent
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@246
PS1, Line 246: ;
.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@247
PS1, Line 247: last_scan_range_submitted_
Not sure I understand why this is needed, since you can infer it from 'remaining_scan_range_submissions_'. I guess to make the condition variable pattern more obvious instead of doing something weird like setting the atomic, then acquiring the lock?
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@317
PS1, Line 317: ScanRangeSharedState shared_state_;
I'm a little skeptical about introducing mutable state into the PlanNode structure - the PlanNode has the right cardinality, but it would be nice to keep execution state out of it.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@671
PS1, Line 671: seperate
nit: separate
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@766
PS1, Line 766: inline ProgressUpdater& progress() { return shared_state_->progress(); }
Consider inlining this function - the callsites might actually be easier to understand if it's explicit that it's updating shared_state_
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.h@830
PS1, Line 830: /// Fetches the next range in queue to read.
nit: extra space after ///. I think the comment could be fleshed out a little, e.g. why it's overridden by subclasses, any invariants
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@242
PS1, Line 242: for (auto ctx : instance_ctxs) {
It's kinda weird that we split up the scan ranges between instances then merge them back together again. I think this is fine since we still need to do that for other scan nodes and we don't want to invent a different place to push hdfs scan ranges.
We should probably rip out the LPT algorithm in Scheduler::AssignRangesToInstances() and just split them evenly using a simpler algorithm, to keep the code simpler and faster. Could be a follow-up patch.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@272
PS1, Line 272: NULL
nit: nullptr while you're here
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@315
PS1, Line 315: Setup
nit: Set up
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@321
PS1, Line 321: // Distribute files evenly among all instances.
This is just to distribute the work for issuing initial scan ranges, right? Comment could be clearer
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@327
PS1, Line 327: DCHECK_GT(instance_ctxs.size(), 0);
This code feels more complex than it needs to be - it has a bunch of edge cases. AFAICT it's correct and we have plenty of test coverage, but it would be nice to simplify it. Could we instead construct min(file_descs.size(), instance_ctxs.size()) vectors, then round-robin assign the files?
Or if you wanted to keep the current distribution, I'd find something like this a little easier to follow.
int num_lists = min(file_descs.size(), instance_ctxs.size());
auto fd_it = file_descs.begin();
for (int i = 0; i < num_lists; ++i) {
vector<HdfsFileDesc*>* curr_file_list =
&shared_state_.file_assignment_per_instance_[instance_ctxs[i]
->fragment_instance_id];
for (int j = 0; j < files_per_instance + (i < remainder); ++j) {
curr_file_lists.push_back(fd_it->second);
++fd_it;
}
}
DCHECK(fd_it == file_descs.end());
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@354
PS1, Line 354: RawValue::Write(eval->GetValue(NULL), template_tuple, slot_desc, NULL);
nit: nullptr
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1226
PS1, Line 1226: while (remaining_scan_range_submissions_.Load() > 0) {
Doesn't this block any instances from processing scan ranges until all have issued their ranges? This doesn't seem to match the function comment exactly. I didn't see a reason why we wouldn't want fragment instances to start processing ranges ASAP. Maybe we can restructure a bit.
I think it's also best to avoid using both last_scan_range_submitted_ and remaining_scan_range_submissions_ in the loop, since they're essentially synonyms. We could also get rid of last_scan_range_submitted_, I think - I left a comment in the header.
It would also be nice if we didn't acquire scan_range_submission_lock_ on every call.
while (true) {
*scan_range = scan_range_queue_.Dequeue();
if (*scan_range != nullptr) return Status::OK();
{
unique_lock<mutex> l(scan_range_submission_lock_);
while (scan_range_queue_.empty() && !last_scan_range_submitted_
&& !state->is_cancelled()) {
range_submission_cv_.Wait(l);
}
// No more work to do.
if (scan_range_queue_.empty() && last_scan_range_submitted_) break;
}
if (state->is_cancelled()) return Status::CANCELLED;
}
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1234
PS1, Line 1234: if(state->is_cancelled()) return Status::CANCELLED;
nit: missing space before ( here and below
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1238
PS1, Line 1238: if(state->is_cancelled()) return Status::CANCELLED;
Is this check actually necessary? It's needed in the loop because otherwise we may get stuck indefinitely, but it seems benign otherwise to start processing the scan range and notice cancellation later.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@1244
PS1, Line 1244: state->AddCancellationCV(&scan_range_submission_lock_, &range_submission_cv_);
This is a little janky because we're signalling a query-level CV as part of the finstance-level state cancellation. I think this is probably fine though..
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc
File be/src/exec/hdfs-scan-node-mt.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc@140
PS1, Line 140: shared_state_->EnqueueScanRange(ranges);
Seems weird to pass in the bool on one branch and not the other. Could make more sense to just call EnqueueScanRange once with a local variable at_front.
shared_state_->EnqueueScanRange(ranges, at_front);
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-mt.cc@142
PS1, Line 142: shared_state_->EnqueueScanRange(ranges, true /*at front*/);
DCHECK that it's TAIL on the else branch
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc@73
PS1, Line 73: HdfsScanNode::HdfsScanNode(ObjectPool* pool, const HdfsScanPlanNode& pnode,
Can we add a DCHECK or anything to make sure that there is only one instance of the legacy scan node on a backend? Not sure, but the interaction with shared_state_ might be depending on that.
I don't think it's possible now but there used to be some weird things where we instantiated the legacy scan node for different file formats.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 May 2020 22:03:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15926/1/tests/query_test/test_mt_dop.py
File tests/query_test/test_mt_dop.py:
http://gerrit.cloudera.org:8080/#/c/15926/1/tests/query_test/test_mt_dop.py@110
PS1, Line 110: v
flake8: E131 continuation line unaligned for hanging indent
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 May 2020 21:18:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6156/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 00:49:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6107/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 May 2020 03:37:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5912/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 23:03:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 3:
(1 comment)
I also added an optimization for adding ranges marked to use hdfs cache to the front of the shared queue. I found out that there was a regression of 10% for TPCH q21 and had noticed that a scan node reading lineitem was slow which stood out since it was already being read on other fragments in the plan on the same node, so tried running the test again with this optimization and the results ended up with no significant perf change.
Result without this optimization:
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.01 | +2.80% | 4.38 | +1.15% |
+----------+-----------------------+---------+------------+------------+----------------+
Results after this optimization:
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@242
PS1, Line 242: for (auto ctx : instance_ctxs) {
> It's kinda weird that we split up the scan ranges between instances then me
Done. I initially thought of ripping out the per instance assignment along with this in a separate patch, but didnt realize that kudu and hbase scan nodes still use the per instance assignment. So instead just removed the LPT algo in this itself
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 19:12:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 23:34:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#6).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
- Added missing lifecycle events from MT scan nodes.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 684 insertions(+), 461 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/6
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 22:27:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 18:23:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#3).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 677 insertions(+), 460 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/3
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6168/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 20:25:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#2).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
TODO:
- Do perf analysis to ensure there is no regression
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
16 files changed, 662 insertions(+), 311 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/2
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6185/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 19:04:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 8: Code-Review+2
Rebased. Carrying over Tim's +2
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 23:03:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc@73
PS1, Line 73: HdfsScanNode::HdfsScanNode(ObjectPool* pool, const HdfsScanPlanNode& pnode,
> Something like that, yeah.
added this check to ProcessScanRangesAndInitSharedState
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc
File be/src/scheduling/scheduler-test.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc@725
PS1, Line 725:
> I think a lot of this test is still relevant, I think we just need to updat
I agree the tests definitely make sense, the only reason I removed them was that I felt that now the code was rudimentary enough that adding a unit test seemed like an overkill as opposed to its predecessor (LPT algo) that had a fair bit of complexity to it.
Let me know if you feel strongly about it and I'll add it in.
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc@459
PS3, Line 459: (ranges)
> redundant parens
Done
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc@459
PS3, Line 459: per_instance_ranges[i % num_instances].push_back((ranges)[i]);
> % here could be a bottleneck if there were a lot of ranges. Maybe it is opt
Done
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 00:05:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#5).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 681 insertions(+), 461 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/5
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 9: Code-Review+2
Carrying over +2
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 18:23:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#9).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
- Added missing lifecycle events from MT scan nodes.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 702 insertions(+), 415 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/9
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6170/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 23:42:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#8).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
- Added missing lifecycle events from MT scan nodes.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 696 insertions(+), 412 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/8
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc
File be/src/scheduling/scheduler-test.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc@725
PS1, Line 725: // Test scheduling algorithm for load-balancing scan ranges within a host.
> I agree the tests definitely make sense, the only reason I removed them was
I would prefer to keep the test as a sanity check
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 May 2020 19:24:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Joe McDonnell, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15926
to look at the new patch set (#7).
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
This patch ameliorates intra node execution skew for multithreaded
HDFS scans by implementing a shared queue of scan ranges for all
instances. Some points to highlight:
- The scan node's PlanNode will go through all the TScanRanges
assigned to all instances and aggregate them into file descriptors.
- These files will be statically and evenly distributed among the
instances.
- The instances would then pick up their set of files and issue
initial ranges by adding them to a shared queue.
- Instances would then fetch their next range to read from this
shared queue.
- Other relevant data structures that will also be shared are:
* remaining_scan_range_submissions_
* partition_template_tuple_map_
* per_file_metadata_
- Removed the longest-processing time (LPT) algorithm in the scheduler
which tries to distribute scan load among instances on a host. This
will have no effect after this patch since now the ranges are shared.
- Added missing lifecycle events from MT scan nodes.
This approach guarantees the following:
- All shared data structures are allocated at the FragmentState
level ensuring they'll stick around till the query is closed.
- All instance local buffers for reading a scan range will be
allocated after it has been taken off the shared queue.
- Since the scheduler can assign ranges from the same file to
different instances, aggregating them into file descriptors early
will ensure the initial footer or header range (depending on file
format) for it will only be issued once.
Limitation:
- For HDFS MT scans we lose out on the optimization within
ReaderContext which ensures cached ranges are read first.
- As a compromise, ranges that are marked to use the hdfs
cache are added to the front of the shared queue.
Testing:
- Added a regression test in test_mt_dop which would almost always
fail without this patch
- Added some test coverage for mt scans where parquet files are split
across blocks and scanning tables with mixed file formats.
- Ran core tests on ASAN build
- Ran exhaustive tests
Performance:
No significant perf change.
Ran TPCH (scale 30) with mt_dop set to 4 on a 3 node minicluster on my
desktop.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 5.85 | +0.35% | 4.35 | +0.60% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(30) | TPCH-Q15 | parquet / none / none | 4.46 | 4.30 | +3.63% | 1.64% | 2.23% | 30 | +3.56% | 5.28 | 7.09 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 9.51 | 9.23 | +3.12% | 1.19% | 0.79% | 30 | +2.84% | 6.48 | 11.69 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 2.37 | 2.32 | +1.89% | 3.76% | 3.26% | 30 | +2.16% | 1.97 | 2.06 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 16.84 | 16.55 | +1.77% | 1.24% | 0.57% | 30 | +1.62% | 5.75 | 6.99 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.65 | 3.60 | +1.43% | 2.19% | 1.72% | 30 | +1.39% | 2.53 | 2.79 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 12.64 | 12.48 | +1.33% | 1.51% | 1.53% | 30 | +1.28% | 2.67 | 3.36 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.03% | 1.91% | 1.77% | 30 | +1.48% | 1.98 | 2.16 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.31 | 1.29 | +1.44% | 5.48% | 3.66% | 30 | +0.43% | 1.10 | 1.18 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 5.49 | 5.44 | +0.78% | 0.59% | 1.16% | 30 | +0.95% | 3.49 | 3.27 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 5.40 | 5.35 | +0.83% | 0.95% | 0.95% | 30 | +0.83% | 2.81 | 3.37 |
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.82 | 2.81 | +0.50% | 1.58% | 1.72% | 30 | +0.30% | 1.22 | 1.16 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.43 | 1.42 | +0.64% | 2.42% | 1.96% | 30 | +0.09% | 0.68 | 1.13 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 2.59 | 2.58 | +0.28% | 1.54% | 1.99% | 30 | +0.23% | 1.17 | 0.60 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 2.17 | 2.17 | +0.15% | 3.04% | 3.68% | 30 | +0.15% | 0.30 | 0.18 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.02 | 2.02 | +0.21% | 2.73% | 2.85% | 30 | +0.07% | 0.26 | 0.30 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 4.16 | 4.15 | +0.04% | 4.49% | 4.38% | 30 | -0.03% | -0.13 | 0.03 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 4.75 | 4.76 | -0.28% | 1.34% | 1.47% | 30 | -0.10% | -0.57 | -0.77 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 4.92 | 4.95 | -0.52% | 1.45% | 2.71% | 30 | +0.03% | 0.12 | -0.94 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.82 | 2.85 | -0.78% | 1.50% | 1.68% | 30 | -0.65% | -1.66 | -1.91 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 4.49 | 4.53 | -0.97% | 2.26% | 2.69% | 30 | -0.68% | -0.99 | -1.52 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 10.11 | 10.19 | -0.85% | 3.27% | 3.71% | 30 | -0.84% | -0.85 | -0.94 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 21.75 | 22.28 | -2.37% | 3.51% | 7.00% | 30 | -0.88% | -1.35 | -1.66 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-orc-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node-mt.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.cc
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/scheduling/scheduler-test.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M tests/query_test/test_mt_dop.py
M tests/query_test/test_scanners.py
19 files changed, 693 insertions(+), 408 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/15926/7
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/6152/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 21:12:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15926 )
Change subject: IMPALA-9655: Dynamic intra-node load balancing for HDFS scans
......................................................................
Patch Set 3:
(5 comments)
Looking pretty good..
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node-base.cc@420
PS1, Line 420: ddBytesReadCounters();
> I thought a lot about this and decided to keep this as it is.
WFM
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/exec/hdfs-scan-node.cc@73
PS1, Line 73: HdfsScanNode::HdfsScanNode(ObjectPool* pool, const HdfsScanPlanNode& pnode,
> not sure about the full context, but do you mean that a DCHECK(use_mt_scan_
Something like that, yeah.
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc
File be/src/scheduling/scheduler-test.cc:
http://gerrit.cloudera.org:8080/#/c/15926/1/be/src/scheduling/scheduler-test.cc@725
PS1, Line 725:
I think a lot of this test is still relevant, I think we just need to update it to check # ranges instead of bytes in a couple of places
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc@459
PS3, Line 459: (ranges)
redundant parens
http://gerrit.cloudera.org:8080/#/c/15926/3/be/src/scheduling/scheduler.cc@459
PS3, Line 459: per_instance_ranges[i % num_instances].push_back((ranges)[i]);
% here could be a bottleneck if there were a lot of ranges. Maybe it is optimized to something without a division in the context of the loop, but you could avoid the division with something like this:
idx = (idx + 1 == ranges.size()) ? 0 : idx + 1;
--
To view, visit http://gerrit.cloudera.org:8080/15926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a101d0d98dff6e3779f85bc466e4c0bdb38094b
Gerrit-Change-Number: 15926
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 May 2020 20:28:42 +0000
Gerrit-HasComments: Yes