Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16526 )
Change subject: KUDU-2612 p5 (b): add highest_seen_txn_id into CoordinatorOpResultPB ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc File src/kudu/transactions/txn_status_manager.cc: http://gerrit.cloudera.org:8080/#/c/16526/1/src/kudu/transactions/txn_status_manager.cc@121 PS1, Line 121: std::lock_guard<simple_spinlock> l(lock_); : if (highest_seen_txn_id) { : *highest_seen_txn_id = highest_txn_id_; : } > Could we only take the lock if 'highest_seen_txn_id' is set? Yep, taking a lock only when highest_seen_txn_id != nullptr is a good idea. Done. The idea is to populate the 'highest_seen_txn_id' parameter in case of success, InvalidArgument, and other cases to keep to TxnManger updated and make the logic there simpler. We always can report back highest_txn_id from there because we know it. Populating the 'highest_seen_txn_id' output parameter in the success case is also important in case of higher contention: e.g., there might be another call to this method that increments highest_txn_id_ before control reaches the end of this method for current thread. I think we could switch to an atomic for 'highest_txn_id_' to avoid holding a lock in the scoped cleanup if we find that it's a bottleneck. I guess we first need to add a small benchmark test for that to make an informed decision. -- To view, visit http://gerrit.cloudera.org:8080/16526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifcb4d90bc10a5695c3f54229688ccdcaf56011d0 Gerrit-Change-Number: 16526 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 02 Oct 2020 08:12:21 +0000 Gerrit-HasComments: Yes
