Dan Burkert has posted comments on this change. Change subject: Replace uses of gutil/atomicops with c++11 atomics ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 318: if (next_rowset_idx_.compare_exchange_weak(current_rowset_idx, rowset_meta_idx)) { > this should be under a lock and not require any atomics next_rowset_idx_ is accessed in CreateRowSet without taking the lock. Line 599: Schema* old_schema = schema_.exchange(new_schema.release()); > it's not really perf sensitive here so not a big deal, but memory_order_seq My comment is saying that it is needed here in order to guarantee that all initialization is complete before another thread tries to access it, is that not correct? http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/util/rwc_lock-test.cc File src/kudu/util/rwc_lock-test.cc: Line 90: while (!state->stop.load()) { > should be relaxed (otherwise we're getting an extra barrier here that might I've changed the two that were marked NoBarrier to memory_order_relaxed, but not the others. http://gerrit.cloudera.org:8080/#/c/1842/1/src/kudu/util/spinlock_profiling.cc File src/kudu/util/spinlock_profiling.cc: Line 280: CHECK(!g_profiling_enabled.exchange(true)); > this is changing the semantics so it doesn't allow recursive enter/exit, le Done -- To view, visit http://gerrit.cloudera.org:8080/1842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249b64ff5efd6c8acbd2c6ff0811f66ff2c4d73f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
