Henry Robinson has posted comments on this change. Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting() ......................................................................
Patch Set 1: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 122: public: indentation http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: Line 123: ReservationTrackerCounters* new_counters = new ReservationTrackerCounters; is there an compiler-generated copy-c'tor for ReservationTrackerCounters? PS1, Line 198: delete it's really tempting to have the AtomicPtr delete its contents on destruction, but I guess we should make a separate AtomicScopedPtr if we have more use cases for it. PS1, Line 307: gc_functions_[i](); if these are caller-supplied, should the interface say something about how they should be non-blocking, since they're executed while holding a spinlock? http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: PS1, Line 386: Must be read and written atomically I'd remove this. Or say "may be read concurrently". It's not clear how a reader could arrange to read this atomically (unless it CAS-es the pointer out of the AtomicPtr?), and I'm not sure that's what the comment means to say. -- To view, visit http://gerrit.cloudera.org:8080/6502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-HasComments: Yes
