Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8758 )
Change subject: IMPALA-6190/6246: Add instances tab and event sequence ...................................................................... Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/8758/2/be/src/runtime/fragment-instance-state.h@169 PS2, Line 169: std::atomic<TFInstanceState::type> current_state_; Should document why this is atomic -- which threads read and which write it. Also, rather than using std::atomic, I think it'd be better to stick with our atomics (until we decide if/when to switch wholesale). You could create a AtomicEnum<> similar to AtomicPtr<>. What we don't like about std::atomic is that it has operator overloads, and using those makes it hard to read code because you don't get any clue in the code sequence itself that atomics are in play (you have to know the types of the variables). -- To view, visit http://gerrit.cloudera.org:8080/8758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I626456b6afa9101eeeeffd5cda10c4096d63d7f9 Gerrit-Change-Number: 8758 Gerrit-PatchSet: 2 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Tue, 05 Dec 2017 22:47:51 +0000 Gerrit-HasComments: Yes
