New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/instrumented mutex wrapper #20726
Conversation
9f5b86b
to
31fd99f
Compare
- agent | ||
- single | ||
description: | | ||
Counts the exclusively locked collection across all shards in a database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counts the exclusively locked collection across all shards in a database. | |
Counts the exclusively locked collections across all shards in a database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also note that on a DB server, the number reported by the metric is the number of exclusively locked shards, not collections.
@@ -0,0 +1,14 @@ | |||
name: arangodb_vocbase_meta_collection_lock_locked_exclusive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should name a publicly visible metric something like "vocbase". "vocbase" is some internal term that doesn't make much sense to end users. The same is true for "meta_collection".
Why not go with some simpler name such as "arangodb_collection_locks_locked_*"? If that name is already in use, maybe some similar easy alternative?
- agent | ||
- single | ||
description: | | ||
Counts the shared locked collection across all shards in a database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counts the shared locked collection across all shards in a database. | |
Counts the shared locked collections across all shards in a database. |
arangod/Metrics/CMakeLists.txt
Outdated
@@ -14,7 +14,8 @@ add_library(arango_metrics STATIC | |||
MetricsFeature.cpp | |||
ClusterMetricsFeature.cpp | |||
${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestMetricsHandler.cpp | |||
${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestUsageMetricsHandler.cpp) | |||
${PROJECT_SOURCE_DIR}/arangod/RestHandler/RestUsageMetricsHandler.cpp | |||
InstrumentedMutex.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the indentation here is somewhat unexpected, but it doesn't matter much.
arangod/Metrics/InstrumentedMutex.h
Outdated
|
||
template<typename F, typename Duration> | ||
auto try_lock_shared_for(Mutex& m, Duration d, F&& fn) requires requires { | ||
m.try_lock_for(d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m.try_lock_for(d); | |
m.try_lock_shared_for(d); |
} else { | ||
lockGuard.cancel(); | ||
if (!_patchCount.empty() && _patchCount == cname) { | ||
auto [guard, res] = co_await rcoll->lockExclusive(to); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this leads to changed behavior.
In the old implementation, the lock acquired by rcoll->lockWrite(to)
acquired on line old:269 could be held until the end of the entire code block. The lock may have been released only on line old:289.
In the new implementation, we are acquiring the lock in line 268 and release it on line 273 already, which is way earlier than before.
So with the new implementation, even if the lock acquistion is successful, we don't execute the following code under the lock, but previously we did:
// re-acquire the mutex. we need it for checking and modifying _iterators.
writeLocker.lock();
it = _iterators.find(cid);
if (it !=
_iterators.end()) { // someone else inserted the iterator in-between
co_return std::make_tuple(Result{}, it->second->logical->id(),
it->second->numberDocuments);
}
numberDocuments = rcoll->meta().numberDocuments();
lazyCreateSnapshot();
@@ -177,5 +178,8 @@ class RocksDBTransactionCollection : public TransactionCollection { | |||
|
|||
bool _usageLocked; | |||
bool _exclusiveWrites; | |||
std::variant<std::monostate, RocksDBMetaCollection::ExclusiveLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: as we are now using std::variant in this header file, we should also add #include <variant>
at the top of the file.
lib/Basics/FutureSharedLock.h
Outdated
|
||
void unlock_shared(LockGuard&& guard) { guard.unlock(); } | ||
|
||
bool owns_lock(LockGuard& guard) { return guard.isLocked(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
bool owns_lock(LockGuard& guard) { return guard.isLocked(); } | |
bool owns_lock(LockGuard const& guard) const noexcept { return guard.isLocked(); } |
InstrumentedMutex<std::shared_mutex> m{metrics}; | ||
|
||
ASSERT_EQ(lockExclusive.load(), 0); | ||
ASSERT_EQ(pendingExclusive.load(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could check here that lockShared
is 0.
ASSERT_TRUE(guard); | ||
|
||
ASSERT_EQ(lockExclusive.load(), 1); | ||
ASSERT_EQ(pendingExclusive.load(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could check here that lockShared
is still 0.
guard.unlock(); | ||
ASSERT_FALSE(guard.owns_lock()); | ||
ASSERT_EQ(lockExclusive.load(), 0); | ||
ASSERT_EQ(pendingExclusive.load(), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
db._createDatabase(database); | ||
db._useDatabase(database); | ||
db._create(collection, {numberOfShards: 1, replicationFactor: 1}); | ||
waitForShardsInSync(collection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: with numberOfShards == 1 && replicationFactor == 1, I don't think we need to wait for any shard to get into sync.
|
||
testSharedLock: function () { | ||
{ | ||
assertMetrics({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this entire test can spuriously fail if there is a write operation ongoing in some other collection in the background. During the tests, this can possibly happen due to some statistics background thread writing data into any of the statistics collections or running a cleanup query on them.
What we could do to work around interferences by temporary background operations is to add a few retries to assertMetrics
, until the desired target state has been reached or some timeout has been exceeded.
|
||
testExclusiveLock: function () { | ||
{ | ||
assertMetrics({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue here as mentioned for above test.
@@ -2058,11 +2058,6 @@ Result RocksDBEngine::dropCollection(TRI_vocbase_t& vocbase, | |||
bool const prefixSameAsStart = true; | |||
bool const useRangeDelete = rcoll->meta().numberDocuments() >= 32 * 1024; | |||
|
|||
auto resLock = rcoll->lockWrite().get(); // technically not necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the comments mentions that this is "technically not necessary", acquiring the lock here may previously have led to synchronization with all ongoing write operations to the collection being fully finished.
With the lock being removed here now, that behavior may or may not change. I can't tell.
To be on the safe side, we could add the lock back even if we can't prove it is actually needed. Or we try and see.
Scope & Purpose
This PR adds an
InstrumentedMutex
wrapper class that collects metrics about the mutex it instruments. These metrics contain:Later I want to add Histograms tracking the lock and wait time. But this PR is already big enough.
As an application the exclusive lock in the
RocksDBMetaCollection
has been instrumented and new metrics were introduced. As a side effect, the locking methods now return a guard object that is stored in theRocksDBTransactionCollection
object.This guard object is necessary for reconstructing the lock times. Otherwise, one can not assign the lock and unlock of a shared lock.