Csaba Ringhofer 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 6: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.h@209 PS6, Line 209: virtual bool LimitCheckedFromMultipleThreads() const { return false; } : virtual bool IsTaskBasedMultiThreadingSupport() const { return false; } optional: maybe creating an enum like ThreadingModel would be better to express this? e.g. SINGLE_THREADED, NON_TASK_BASED_SCANNER, TASK_BASED_SCANNER. http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.h@277 PS6, Line 277: /// Caps the input row batch to ensure that the limit is not exceeded. : /// Sets the eos and returns true, if the limit is reached. : bool CheckLimitAndTruncateRowBatchIfNeeded(RowBatch* row_batch, bool* eos); : : /// Caps the input row batch to ensure that the limit is not exceeded. : /// Sets the eos and returns true, if the limit is reached. : /// Uses thread safe functions. : bool CheckLimitAndTruncateRowBatchIfNeededShared(RowBatch* row_batch, bool* eos); These could be moved to "protected". Can you check other functions too and make them protected, unless other classes use them? http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.cc@418 PS6, Line 418: (limit_ == -1 || (rows_returned() + row_batch_size) < limit_) Same as line 436. http://gerrit.cloudera.org:8080/#/c/13178/6/be/src/exec/exec-node.cc@436 PS6, Line 436: (limit_ == -1 || (rows_returned_shared() + row_batch_size) < limit_) 'reached_limit' could be set to this value from the start. It could be also reused instead of calling ReachedLimitShared() - we already assume that no other thread changes num_rows_returned_. -- 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: 6 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: Mon, 06 May 2019 13:51:07 +0000 Gerrit-HasComments: Yes
