Alexey Serbin has posted comments on this change. Change subject: [c++ client] implemented session operations stats ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/4974/2/src/kudu/client/write_op_stats.h File src/kudu/client/write_op_stats.h: PS2, Line 37: IncTotal > Nit: suggest using a nomenclature which could reflect that this is an atomi Good point -- I'll add a comment on that describing why atomics are used, but I don't think the signature should reflect that. In my opinion, the interface of this simple class should not expose the implementation details (so-called encapsulation). The atomics here are used only because those counters could be updated from multiple places -- both from the main thread where the KuduSession::Apply() is called and from the background reactor threads which can call these methods (actually, only IncFailed()) with the results of the RPC call. PS2, Line 37: ++total_ > The standard seem to suggest that ++ operation follows strict synchronizati Yes, good point. PS2, Line 40: total_ > I wonder if this is equivalent to total_.load(memory_order_relaxed) ? I think that due to the consistency it should be an operation with memory_order_seq_cst. You can the details of the implementation looking into template atomic::<T>::operator T() method. PS2, Line 49: total_count_ > Do you want to point out where is this variable declared ? Done -- To view, visit http://gerrit.cloudera.org:8080/4974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifb4191105158f49d811763fdc8e82831b4e2e0be Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
