Tim Armstrong has posted comments on this change. Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting() ......................................................................
Patch Set 1: Code-Review+1 (5 comments) Carry +1 http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 122: public: > indentation Done 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? Done PS1, Line 198: delete > it's really tempting to have the AtomicPtr delete its contents on destructi I thought about that too, but it complicates the implementation a bit to have an interface analogous to scoped_ptr - e.g. you have to implement a Store() as a swap and delete to handle the case when the value is already non-null. PS1, Line 307: gc_functions_[i](); > if these are caller-supplied, should the interface say something about how I added a bit more description of what the gc_functions_ should do. The current SpinLock implementation now uses gutil's spinlock, which falls back to a kernel futex after a while, so we don't have the same problem as with the old SpinLock. That said, mutex is still probably a better fit here, since the critical section is pretty long-running and it shouldn't be frequently acquired. The fairness properties may be nice too. So I reverted that part of the change. 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 re Done. -- 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-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
