This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new ac391b6 [transactions] remove cached TransactionEntry's state
ac391b6 is described below
commit ac391b621649e85cd202e53437a63cfbdedf5179
Author: Alexey Serbin <[email protected]>
AuthorDate: Wed Jan 6 17:37:13 2021 -0800
[transactions] remove cached TransactionEntry's state
With changelist fa21bf618, an atomic field was added to TransactionEntry
to contain a cached state for an entry. The motivation was to provide
lock-free access the state of the entry: accessing the metadata stored
as a COW object in the 'metadata_' field involves taking read lock
for that. The transaction keepalive tracking task needs to check the
status of an entry every time it runs (10 seconds by default), and
there may be many txn records records.
However, that approach didn't provide an automatic update of the cached
field upon modification of the underlying 'metadata_' object, and
updating that manually proved to be cumbersome and bug prone. A few
alternatives to update the cached field automatically were considered,
but after realizing that we don't have hard data showing a lot of
lock contention or resources spent on lock acquisition, the former
approach was deemed to be a premature optimization.
With this changelist, the cached field is removed and the
TransactionEntry::state() accessor now takes a read lock
to access the state of the transactional entry.
Change-Id: Iab1ffe4312d21b732d7f37f7b54f28e43c979e35
Reviewed-on: http://gerrit.cloudera.org:8080/16928
Reviewed-by: Andrew Wong <[email protected]>
Tested-by: Kudu Jenkins
---
src/kudu/transactions/txn_status_entry.cc | 8 ++++++--
src/kudu/transactions/txn_status_entry.h | 19 +++----------------
src/kudu/transactions/txn_status_manager.cc | 4 ----
3 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/src/kudu/transactions/txn_status_entry.cc
b/src/kudu/transactions/txn_status_entry.cc
index 7170a0f..48f74d2 100644
--- a/src/kudu/transactions/txn_status_entry.cc
+++ b/src/kudu/transactions/txn_status_entry.cc
@@ -36,8 +36,7 @@ namespace transactions {
TransactionEntry::TransactionEntry(int64_t txn_id, std::string user)
: txn_id_(txn_id),
user_(std::move(user)),
- last_heartbeat_time_(MonoTime::Now()),
- state_(TxnStatePB::UNKNOWN) {
+ last_heartbeat_time_(MonoTime::Now()) {
}
scoped_refptr<ParticipantEntry> TransactionEntry::GetOrCreateParticipant(
@@ -62,5 +61,10 @@ vector<string> TransactionEntry::GetParticipantIds() const {
return ret;
}
+TxnStatePB TransactionEntry::state() const {
+ CowLock<PersistentTransactionEntry> l(&metadata_, LockMode::READ);
+ return l.data().pb.state();
+}
+
} // namespace transactions
} // namespace kudu
diff --git a/src/kudu/transactions/txn_status_entry.h
b/src/kudu/transactions/txn_status_entry.h
index c96c0f8..fe25409 100644
--- a/src/kudu/transactions/txn_status_entry.h
+++ b/src/kudu/transactions/txn_status_entry.h
@@ -23,8 +23,6 @@
#include <utility>
#include <vector>
-#include <glog/logging.h>
-
#include "kudu/gutil/ref_counted.h"
#include "kudu/transactions/transactions.pb.h"
#include "kudu/util/cow_object.h"
@@ -101,13 +99,9 @@ class TransactionEntry : public
RefCountedThreadSafe<TransactionEntry> {
last_heartbeat_time_.store(hbtime);
}
- TxnStatePB state() const {
- return state_.load();
- }
- void SetState(TxnStatePB state) {
- DCHECK(metadata_.IsWriteLocked());
- state_.store(state);
- }
+ // An accessor to the transaction's state. Concurrent access is controlled
+ // via the locking provided by the underlying copy-on-write metadata_ object.
+ TxnStatePB state() const;
private:
friend class RefCountedThreadSafe<TransactionEntry>;
@@ -135,13 +129,6 @@ class TransactionEntry : public
RefCountedThreadSafe<TransactionEntry> {
// different clients sending writes in the context of the same
transaction
// * the field is read by the stale transaction tracker in TxnStatusManager
std::atomic<MonoTime> last_heartbeat_time_;
-
- // A shortcut for the state of the transaction stored in the metadata_ field.
- // Callers should externally synchronize 'state_' with 'metadata_' by calling
- // SetState() accordingly when TxnStatePB in 'metadata_' changes.
- //
- // TODO(aserbin): add a hook into CowObject::Commit() to do so automatically
- std::atomic<TxnStatePB> state_;
};
typedef MetadataLock<TransactionEntry> TransactionEntryLock;
diff --git a/src/kudu/transactions/txn_status_manager.cc
b/src/kudu/transactions/txn_status_manager.cc
index a7f877a..8062df6 100644
--- a/src/kudu/transactions/txn_status_manager.cc
+++ b/src/kudu/transactions/txn_status_manager.cc
@@ -286,7 +286,6 @@ Status TxnStatusManager::BeginTransaction(int64_t txn_id,
TransactionEntryLock txn_lock(txn.get(), LockMode::WRITE);
txn_lock.mutable_data()->pb.set_state(TxnStatePB::OPEN);
txn_lock.mutable_data()->pb.set_user(user);
- txn->SetState(txn_lock.data().pb.state());
txn_lock.Commit();
}
std::lock_guard<simple_spinlock> l(lock_);
@@ -319,7 +318,6 @@ Status TxnStatusManager::BeginCommitTransaction(int64_t
txn_id, const string& us
auto* mutable_data = txn_lock.mutable_data();
mutable_data->pb.set_state(TxnStatePB::COMMIT_IN_PROGRESS);
RETURN_NOT_OK(status_tablet_.UpdateTransaction(txn_id, mutable_data->pb,
ts_error));
- txn->SetState(txn_lock.data().pb.state());
txn_lock.Commit();
return Status::OK();
}
@@ -346,7 +344,6 @@ Status TxnStatusManager::FinalizeCommitTransaction(
mutable_data->pb.set_state(TxnStatePB::COMMITTED);
RETURN_NOT_OK(status_tablet_.UpdateTransaction(
txn_id, mutable_data->pb, ts_error));
- txn->SetState(txn_lock.data().pb.state());
txn_lock.Commit();
return Status::OK();
}
@@ -373,7 +370,6 @@ Status TxnStatusManager::AbortTransaction(int64_t txn_id,
auto* mutable_data = txn_lock.mutable_data();
mutable_data->pb.set_state(TxnStatePB::ABORTED);
RETURN_NOT_OK(status_tablet_.UpdateTransaction(txn_id, mutable_data->pb,
ts_error));
- txn->SetState(txn_lock.data().pb.state());
txn_lock.Commit();
return Status::OK();
}
