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
commit a3ed48d932d203e9310cca396e5530d6ed6eaa87
Author: Alexey Serbin <ale...@apache.org>
AuthorDate: Wed May 8 15:51:27 2024 -0700
[common] switch from unique_lock to lock_guard
To simplify working with STL synchronization primitives, this patch
changes from std::unique_lock to std::lock_guard where appropriate.
In addition, IWYU and ClangTidy's feedback has been addressed.
Change-Id: I72d86ca730113ec0652154d5ce509fc2e479befb
Reviewed-on: http://gerrit.cloudera.org:8080/21414
Tested-by: Alexey Serbin <ale...@apache.org>
Reviewed-by: Zoltan Chovan <zcho...@cloudera.com>
Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com>
---
src/kudu/clock/hybrid_clock.cc | 2 +-
src/kudu/consensus/consensus_peers.cc | 12 +++++++-----
src/kudu/consensus/consensus_queue.cc | 8 ++++----
src/kudu/consensus/log_cache.cc | 2 +-
src/kudu/integration-tests/tombstoned_voting-stress-test.cc | 4 ++--
src/kudu/rpc/rpcz_store.cc | 2 +-
src/kudu/security/tls_context.cc | 9 ++++-----
src/kudu/server/webserver.cc | 2 +-
src/kudu/tablet/lock_manager-test.cc | 5 +++--
src/kudu/tablet/lock_manager.cc | 11 ++++++-----
src/kudu/tablet/tablet.cc | 4 +++-
src/kudu/tserver/scanners.cc | 2 +-
src/kudu/util/maintenance_manager.cc | 2 +-
src/kudu/util/trace_metrics.h | 2 +-
14 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/src/kudu/clock/hybrid_clock.cc b/src/kudu/clock/hybrid_clock.cc
index f9c0a7816..1f2eae483 100644
--- a/src/kudu/clock/hybrid_clock.cc
+++ b/src/kudu/clock/hybrid_clock.cc
@@ -709,7 +709,7 @@ Status HybridClock::WalltimeWithError(uint64_t* now_usec,
uint64_t* error_usec)
MonoTime read_time_max_likelihood = read_time_before +
MonoDelta::FromMicroseconds(read_time_error_us);
- std::unique_lock<decltype(last_clock_read_lock_)> l(last_clock_read_lock_);
+ std::lock_guard<decltype(last_clock_read_lock_)> l(last_clock_read_lock_);
if (is_extrapolating_) {
is_extrapolating_ = false;
extrapolating_->set_value(is_extrapolating_);
diff --git a/src/kudu/consensus/consensus_peers.cc
b/src/kudu/consensus/consensus_peers.cc
index 27dd2a1c6..492d417b9 100644
--- a/src/kudu/consensus/consensus_peers.cc
+++ b/src/kudu/consensus/consensus_peers.cc
@@ -24,6 +24,7 @@
#include <mutex>
#include <ostream>
#include <string>
+#include <type_traits>
#include <vector>
#include <gflags/gflags.h>
@@ -334,7 +335,7 @@ void Peer::StartElection() {
void Peer::ProcessResponse() {
// Note: This method runs on the reactor thread.
- std::unique_lock<simple_spinlock> lock(peer_lock_);
+ std::lock_guard<simple_spinlock> lock(peer_lock_);
if (PREDICT_FALSE(closed_)) {
return;
}
@@ -416,7 +417,7 @@ void Peer::DoProcessResponse() {
queue_->ResponseFromPeer(peer_pb_.permanent_uuid(), response_);
{
- std::unique_lock<simple_spinlock> lock(peer_lock_);
+ std::lock_guard<simple_spinlock> lock(peer_lock_);
CHECK(request_pending_);
failed_attempts_ = 0;
request_pending_ = false;
@@ -457,12 +458,13 @@ void Peer::ProcessTabletCopyResponse() {
queue_->UpdatePeerStatus(peer_pb_.permanent_uuid(), PeerStatus::OK,
Status::OK());
} else if (!tc_response_.has_error() ||
tc_response_.error().code() !=
TabletServerErrorPB::TabletServerErrorPB::THROTTLED) {
+ const auto& response_str = controller_status.ok()
+ ? SecureShortDebugString(tc_response_) : controller_status.ToString();
+ lock.unlock();
// THROTTLED is a common response after a tserver with many replicas fails;
// logging it would generate a great deal of log spam.
LOG_WITH_PREFIX_UNLOCKED(WARNING) << "Unable to start Tablet Copy on peer:
"
- << (controller_status.ok() ?
- SecureShortDebugString(tc_response_)
:
- controller_status.ToString());
+ << response_str;
}
}
diff --git a/src/kudu/consensus/consensus_queue.cc
b/src/kudu/consensus/consensus_queue.cc
index 86a7a3eda..dd0f5fe53 100644
--- a/src/kudu/consensus/consensus_queue.cc
+++ b/src/kudu/consensus/consensus_queue.cc
@@ -459,7 +459,7 @@ void PeerMessageQueue::TruncateOpsAfter(int64_t index) {
LogPrefixUnlocked(),
index));
{
- std::unique_lock<simple_spinlock> lock(queue_lock_);
+ std::lock_guard<simple_spinlock> lock(queue_lock_);
DCHECK(op.IsInitialized());
queue_state_.last_appended = op;
}
@@ -467,13 +467,13 @@ void PeerMessageQueue::TruncateOpsAfter(int64_t index) {
}
OpId PeerMessageQueue::GetLastOpIdInLog() const {
- std::unique_lock<simple_spinlock> lock(queue_lock_);
+ std::lock_guard<simple_spinlock> lock(queue_lock_);
DCHECK(queue_state_.last_appended.IsInitialized());
return queue_state_.last_appended;
}
OpId PeerMessageQueue::GetNextOpId() const {
- std::unique_lock<simple_spinlock> lock(queue_lock_);
+ std::lock_guard<simple_spinlock> lock(queue_lock_);
DCHECK(queue_state_.last_appended.IsInitialized());
return MakeOpId(queue_state_.current_term,
queue_state_.last_appended.index() + 1);
@@ -958,7 +958,7 @@ void
PeerMessageQueue::UpdateLastIndexAppendedToLeader(int64_t last_idx_appended
void PeerMessageQueue::UpdatePeerStatus(const string& peer_uuid,
PeerStatus ps,
const Status& status) {
- std::unique_lock<simple_spinlock> l(queue_lock_);
+ std::lock_guard<simple_spinlock> l(queue_lock_);
TrackedPeer* peer = FindPtrOrNull(peers_map_, peer_uuid);
if (PREDICT_FALSE(peer == nullptr || queue_state_.mode == NON_LEADER)) {
VLOG(1) << LogPrefixUnlocked() << "peer " << peer_uuid
diff --git a/src/kudu/consensus/log_cache.cc b/src/kudu/consensus/log_cache.cc
index 03f67fc7a..8ed7105c1 100644
--- a/src/kudu/consensus/log_cache.cc
+++ b/src/kudu/consensus/log_cache.cc
@@ -125,7 +125,7 @@ void LogCache::Init(const OpId& preceding_op) {
}
void LogCache::TruncateOpsAfter(int64_t index) {
- std::unique_lock<simple_spinlock> l(lock_);
+ std::lock_guard<simple_spinlock> l(lock_);
TruncateOpsAfterUnlocked(index);
}
diff --git a/src/kudu/integration-tests/tombstoned_voting-stress-test.cc
b/src/kudu/integration-tests/tombstoned_voting-stress-test.cc
index a092a716d..c61099f28 100644
--- a/src/kudu/integration-tests/tombstoned_voting-stress-test.cc
+++ b/src/kudu/integration-tests/tombstoned_voting-stress-test.cc
@@ -138,7 +138,7 @@ string TombstonedVotingStressTest::State_Name(State state) {
}
TombstonedVotingStressTest::State TombstonedVotingStressTest::GetState() {
- unique_lock<Mutex> l(lock_);
+ std::lock_guard<Mutex> l(lock_);
bool blocked = false;
if (block_workers_) {
num_workers_blocked_++;
@@ -160,7 +160,7 @@ void TombstonedVotingStressTest::SetState(State state) {
// 3. Change state.
// 4. Unblock workers.
LOG(INFO) << "setting state to " << State_Name(state);
- unique_lock<Mutex> l(lock_);
+ std::lock_guard<Mutex> l(lock_);
block_workers_ = true;
while (num_workers_blocked_ != num_workers_) {
cond_all_workers_blocked_.Wait();
diff --git a/src/kudu/rpc/rpcz_store.cc b/src/kudu/rpc/rpcz_store.cc
index d2425e52a..513c88840 100644
--- a/src/kudu/rpc/rpcz_store.cc
+++ b/src/kudu/rpc/rpcz_store.cc
@@ -207,7 +207,7 @@ void MethodSampler::GetSamplePBs(RpczMethodPB* method_pb) {
}
auto* sample_pb = method_pb->add_samples();
- std::unique_lock<simple_spinlock> lock(bucket.sample_lock);
+ std::lock_guard<simple_spinlock> lock(bucket.sample_lock);
sample_pb->mutable_header()->CopyFrom(bucket.sample.header);
sample_pb->set_trace(bucket.sample.trace->DumpToString(Trace::INCLUDE_TIME_DELTAS));
diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc
index c28647587..5a00b7153 100644
--- a/src/kudu/security/tls_context.cc
+++ b/src/kudu/security/tls_context.cc
@@ -93,7 +93,6 @@ using std::nullopt;
using std::optional;
using std::shared_lock;
using std::string;
-using std::unique_lock;
using std::vector;
using strings::Substitute;
@@ -369,7 +368,7 @@ Status TlsContext::UseCertificateAndKey(const Cert& cert,
const PrivateKey& key)
// Verify that the cert and key match.
RETURN_NOT_OK(cert.CheckKeyMatch(key));
- std::unique_lock<RWMutex> lock(lock_);
+ std::lock_guard<RWMutex> lock(lock_);
// Verify that the appropriate CA certs have been loaded into the context
// before we adopt a cert. Otherwise, client connections without the CA cert
@@ -406,7 +405,7 @@ Status TlsContext::AddTrustedCertificate(const Cert& cert) {
CHECK_OK(cert.GetPublicKey(&k));
}
- unique_lock<RWMutex> lock(lock_);
+ std::lock_guard<RWMutex> lock(lock_);
auto* cert_store = SSL_CTX_get_cert_store(ctx_.get());
// Iterate through the certificate chain and add each individual certificate
to the store.
@@ -530,7 +529,7 @@ Status TlsContext::GenerateSelfSignedCertAndKey() {
ERR_clear_error(); // in case it left anything on the queue.
// Step 4: Adopt the new key and cert.
- unique_lock<RWMutex> lock(lock_);
+ std::lock_guard<RWMutex> lock(lock_);
CHECK(!has_cert_);
OPENSSL_RET_NOT_OK(SSL_CTX_use_PrivateKey(ctx_.get(), key.GetRawData()),
"failed to use private key");
@@ -552,7 +551,7 @@ optional<CertSignRequest> TlsContext::GetCsrIfNecessary()
const {
Status TlsContext::AdoptSignedCert(const Cert& cert) {
SCOPED_OPENSSL_NO_PENDING_ERRORS;
- unique_lock<RWMutex> lock(lock_);
+ std::lock_guard<RWMutex> lock(lock_);
if (!csr_) {
// A signed cert has already been adopted.
diff --git a/src/kudu/server/webserver.cc b/src/kudu/server/webserver.cc
index b505995a8..696f41591 100644
--- a/src/kudu/server/webserver.cc
+++ b/src/kudu/server/webserver.cc
@@ -527,7 +527,7 @@ int Webserver::LogMessageCallbackStatic(const struct
sq_connection* /*connection
// the squeasel server uses the log callback to report on errors.
{
static simple_spinlock kErrMsgLock_;
- std::unique_lock<simple_spinlock> l(kErrMsgLock_);
+ std::lock_guard<simple_spinlock> l(kErrMsgLock_);
kWebserverLastErrMsg = message;
}
LOG(ERROR) << "Webserver: " << message;
diff --git a/src/kudu/tablet/lock_manager-test.cc
b/src/kudu/tablet/lock_manager-test.cc
index 7ff727a6f..466f5be84 100644
--- a/src/kudu/tablet/lock_manager-test.cc
+++ b/src/kudu/tablet/lock_manager-test.cc
@@ -25,6 +25,7 @@
#include <ostream>
#include <string>
#include <thread>
+#include <type_traits>
#include <vector>
#include <gflags/gflags.h>
@@ -364,7 +365,7 @@ class LmTestResource {
Slice id() const { return id_; }
void acquire(uint64_t tid) {
- std::unique_lock<std::mutex> lock(lock_);
+ std::lock_guard<std::mutex> lock(lock_);
CHECK(!is_owned_);
CHECK_EQ(0, owner_);
owner_ = tid;
@@ -372,7 +373,7 @@ class LmTestResource {
}
void release(uint64_t tid) {
- std::unique_lock<std::mutex> lock(lock_);
+ std::lock_guard<std::mutex> lock(lock_);
CHECK(is_owned_);
CHECK_EQ(tid, owner_);
owner_ = 0;
diff --git a/src/kudu/tablet/lock_manager.cc b/src/kudu/tablet/lock_manager.cc
index 90bfe9ef7..4b8289767 100644
--- a/src/kudu/tablet/lock_manager.cc
+++ b/src/kudu/tablet/lock_manager.cc
@@ -17,6 +17,8 @@
#include "kudu/tablet/lock_manager.h"
+#include <sys/types.h>
+
#include <cstddef>
#include <limits>
#include <memory>
@@ -47,7 +49,6 @@
using kudu::tserver::TabletServerErrorPB;
using std::string;
-using std::unique_lock;
using std::unique_ptr;
using std::vector;
using strings::Substitute;
@@ -197,7 +198,7 @@ vector<LockEntry*>
LockTable::GetLockEntries(ArrayView<Slice> keys) {
// TODO(todd) prefetch the hash buckets
{
- unique_lock<simple_spinlock> l(lock_);
+ std::lock_guard<simple_spinlock> l(lock_);
for (int i = 0; i < entries.size(); i++) {
LockEntry* new_entry = entries[i];
Bucket* bucket = FindBucket(new_entry->key_hash_);
@@ -250,10 +251,10 @@ void LockTable::ReleaseLockEntries(ArrayView<LockEntry*>
entries) {
};
{
- unique_lock<simple_spinlock> l(lock_);
+ std::lock_guard<simple_spinlock> l(lock_);
- auto it = entries.begin();
- int rem = entries.size();
+ const auto* it = entries.cbegin();
+ ssize_t rem = entries.size();
// Manually block the loop into a series of constant-sized batches
// followed by one last variable-sized batch for the remainder.
diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index ca452d210..d6d811d6a 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -2893,8 +2893,10 @@ double
Tablet::GetPerfImprovementForBestDeltaCompact(RowSet::DeltaCompactionType
double
Tablet::GetPerfImprovementForBestDeltaCompactUnlocked(RowSet::DeltaCompactionType
type,
shared_ptr<RowSet>* rs) const {
+#ifndef NDEBUG
std::unique_lock<std::mutex> cs_lock(compact_select_lock_, std::try_to_lock);
- DCHECK(!cs_lock.owns_lock());
+ CHECK(!cs_lock.owns_lock());
+#endif
scoped_refptr<TabletComponents> comps;
GetComponents(&comps);
double worst_delta_perf = 0;
diff --git a/src/kudu/tserver/scanners.cc b/src/kudu/tserver/scanners.cc
index 7bae8cb45..29590b4b3 100644
--- a/src/kudu/tserver/scanners.cc
+++ b/src/kudu/tserver/scanners.cc
@@ -523,7 +523,7 @@ Scanner::~Scanner() {
}
void Scanner::AddTimings(const CpuTimes& elapsed) {
- std::unique_lock<RWMutex> l(cpu_times_lock_);
+ std::lock_guard<RWMutex> l(cpu_times_lock_);
cpu_times_.Add(elapsed);
}
diff --git a/src/kudu/util/maintenance_manager.cc
b/src/kudu/util/maintenance_manager.cc
index 5145cd929..35e0010f2 100644
--- a/src/kudu/util/maintenance_manager.cc
+++ b/src/kudu/util/maintenance_manager.cc
@@ -351,7 +351,7 @@ void MaintenanceManager::RunSchedulerThread() {
MaintenanceOp* op = nullptr;
string op_note;
{
- std::unique_lock<Mutex> guard(lock_);
+ std::lock_guard<Mutex> guard(lock_);
// Upon each iteration, we should have dropped and reacquired 'lock_'.
// Register any ops that may have been buffered for registration while
the
// lock was last held.
diff --git a/src/kudu/util/trace_metrics.h b/src/kudu/util/trace_metrics.h
index 8c460bd2c..00b095eb4 100644
--- a/src/kudu/util/trace_metrics.h
+++ b/src/kudu/util/trace_metrics.h
@@ -77,7 +77,7 @@ inline void TraceMetrics::Increment(const char* name, int64_t
amount) {
}
inline std::map<const char*, int64_t> TraceMetrics::Get() const {
- std::unique_lock<simple_spinlock> l(lock_);
+ std::lock_guard<simple_spinlock> l(lock_);
return counters_;
}
