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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2020 02:45:01 +0000 Gerrit-HasComments: Yes
