Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13178 )
Change subject: IMPALA-4658: Potential race if compiler reorders ReachedLimit() usage. ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/hdfs-scan-node.cc@144 PS4, Line 144: DecrementNumRowsReturnedShared(num_rows_over); Is this logic racy in that multiple threads could see that they pushed the limit over, and then over-compensate? For example, with a limit of 10, and two scanner threads producing batches of 100, they might both increment by 100, see that the total rows returned is 200, and then both want to compensate by removing 190. This particular case ends up setting the rows returned counter to a negative number, but even outside of that particular issue, it seems like we might undershoot the limit http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/hdfs-sequence-scanner.cc File be/src/exec/hdfs-sequence-scanner.cc: http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/hdfs-sequence-scanner.cc@295 PS4, Line 295: scan_node_->limit() - scan_node_->rows_returned_shared(); similar question here about whether this is going to get wrong results around hitting the limit if there are actually multiple threads in this same path (I don't really know much about the MT scanner design) http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/13178/4/be/src/exec/kudu-scan-node.cc@111 PS4, Line 111: int num_rows_over = rows_returned_shared() - limit_; same concern here -- To view, visit http://gerrit.cloudera.org:8080/13178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4cbbfad80f7ab87dd6f192a24e2c68f7c66b047e Gerrit-Change-Number: 13178 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 01 May 2019 22:57:47 +0000 Gerrit-HasComments: Yes
