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 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/13178/5/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/13178/5/be/src/exec/exec-node.h@279 PS5, Line 279: bool CheckLimitAndTruncateRowBatchIfNeeded(RowBatch* row_batch, bool* eos) { We can probably just move these to the .cc file to avoid the additional header dependency - these aren't like enough on the hot path that a direct function call makes a difference. http://gerrit.cloudera.org:8080/#/c/13178/5/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/13178/5/be/src/exec/hdfs-scan-node.cc@136 PS5, Line 136: if (CheckLimitAndTruncateRowBatchIfNeededShared(row_batch, eos)) { The house style is to put conditionals like this on one line if they fit, i.e. if (CheckLimitAndTruncateRowBatchIfNeededShared(row_batch, eos)) SetDone(); -- 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: 5 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 04 May 2019 00:00:17 +0000 Gerrit-HasComments: Yes
