Tim Armstrong 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 2: (6 comments) I did a pass over this. Looks good - just had a couple of suggestions for cleanup. I wanted to do another final pass after follow-on changes just to double-check that the new version was identical to the old version. http://gerrit.cloudera.org:8080/#/c/13178/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13178/2//COMMIT_MSG@46 PS2, Line 46: The scan nodes with task based multi-threading (MT) also use the new This seems reasonable. Ideally we could avoid this for DataSourceScanNode and HBaseScanNode though, since those don't have MT implementations. Maybe we can just make an exception in the DCHECK - we have the TPlanNodeType in the ExecNode. http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h@209 PS2, Line 209: retrurned typo http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h@215 PS2, Line 215: retrurned typo http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h@216 PS2, Line 216: /// rows_returned(). Can you add a short TODO that these can be removed if we remove the legacy multi-threaded scan nodes. http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h@236 PS2, Line 236: resturned typo: returned http://gerrit.cloudera.org:8080/#/c/13178/2/be/src/exec/exec-node.h@391 PS2, Line 391: AtomicInt64 num_rows_returned_shared_; It's a bit of a matter of taste but I think it would be better to just have a single member and use the atomic Acquire_Load, Release_Store, Barrier_AtomicIncrement operations on it directly. It's well-encapsulated in these functions so should be ok -- 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: 2 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 30 Apr 2019 15:47:41 +0000 Gerrit-HasComments: Yes
