Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18929 )
Change subject: IMPALA-11539: Mitigate intra-node skew of HDFS scans with MT_DOP ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@13 PS3, Line 13: for HDFS not just HDFS, right? do we also use the same logic for files in cloud storage? http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@16 PS3, Line 16: thisqueue. typo http://gerrit.cloudera.org:8080/#/c/18929/3//COMMIT_MSG@25 PS3, Line 25: Ranges that are marked to use the hdfs cache are still handled with : higher priority. Probably out of scope for this patch, but data cache / file handle cache could be also considered, as postponing the processing of cached files increases the chance of being evicted in the meantime. e.g. if files have a similar length, then this algorithm creates a fix order for them, which is kind of worst case if the scan doesn't fit to the cache (assuming LRU cache). http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h File be/src/exec/scan-range-queue-mt.h: http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@30 PS3, Line 30: Only used for MT scans where the scan ranges are dynamically assigned : /// to the fragment instances using this queue. Why is it treated differently than the MT_DOP=0 case? I would assume that scanner threads can have similar issues with skew - the only difference is that that number of scanner threads is not fixed, so we cannot do a static assignment, but this algorithm would work without issue IMO. http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@75 PS3, Line 75: scan_range_queue_.Reserve(capacity); This should also take the lock IMO, as reserve can cause the underlying vector to be moved. http://gerrit.cloudera.org:8080/#/c/18929/3/be/src/exec/scan-range-queue-mt.h@83 PS3, Line 83: if (!lhs->UseHdfsCache() && rhs->UseHdfsCache()) return true; Shouldn't we return false in the opposite case? http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py@378 PS3, Line 378: 'text', 'parquet Does it add to the coverage if we execute with both fileformats? My guess would be is that text is more reliably, as the size should be linear to the number of rows. http://gerrit.cloudera.org:8080/#/c/18929/3/tests/query_test/test_scanners.py@419 PS3, Line 419: ' nit: Lets -- To view, visit http://gerrit.cloudera.org:8080/18929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib7dc1f1665565da6c0e155c1e585f7089b18a180 Gerrit-Change-Number: 18929 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Tue, 30 Aug 2022 13:05:44 +0000 Gerrit-HasComments: Yes
