You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2018/01/18 02:57:57 UTC
[1/2] kudu git commit: rpc: avoid an extra copy of shared_ptr for
OutboundCall
Repository: kudu
Updated Branches:
refs/heads/master 6ace408ee -> d3165ed2b
rpc: avoid an extra copy of shared_ptr for OutboundCall
Change-Id: I89356bd9c59e612e64ffc4e1ad4a6bfda44512aa
Reviewed-on: http://gerrit.cloudera.org:8080/9047
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Michael Ho <kw...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/df65df65
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/df65df65
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/df65df65
Branch: refs/heads/master
Commit: df65df65d4f3030d93720e11531d224ff0bcff94
Parents: 6ace408
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Jan 17 15:33:41 2018 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Thu Jan 18 01:26:15 2018 +0000
----------------------------------------------------------------------
src/kudu/rpc/connection.cc | 4 ++--
src/kudu/rpc/connection.h | 2 +-
src/kudu/rpc/reactor.cc | 6 +++---
src/kudu/rpc/reactor.h | 2 +-
4 files changed, 7 insertions(+), 7 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/df65df65/src/kudu/rpc/connection.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/connection.cc b/src/kudu/rpc/connection.cc
index e4a9245..a28eddb 100644
--- a/src/kudu/rpc/connection.cc
+++ b/src/kudu/rpc/connection.cc
@@ -309,7 +309,7 @@ struct CallTransferCallbacks : public TransferCallbacks {
Connection* conn_;
};
-void Connection::QueueOutboundCall(const shared_ptr<OutboundCall> &call) {
+void Connection::QueueOutboundCall(shared_ptr<OutboundCall> call) {
DCHECK(call);
DCHECK_EQ(direction_, CLIENT);
DCHECK(reactor_thread_->IsCurrentThread());
@@ -390,7 +390,7 @@ void Connection::QueueOutboundCall(const shared_ptr<OutboundCall> &call) {
car->timeout_timer.start();
}
- TransferCallbacks *cb = new CallTransferCallbacks(call, this);
+ TransferCallbacks *cb = new CallTransferCallbacks(std::move(call), this);
awaiting_response_[call_id] = car.release();
QueueOutbound(gscoped_ptr<OutboundTransfer>(
OutboundTransfer::CreateForCallRequest(call_id, tmp_slices, n_slices, cb)));
http://git-wip-us.apache.org/repos/asf/kudu/blob/df65df65/src/kudu/rpc/connection.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/connection.h b/src/kudu/rpc/connection.h
index d64e557..4e44132 100644
--- a/src/kudu/rpc/connection.h
+++ b/src/kudu/rpc/connection.h
@@ -121,7 +121,7 @@ class Connection : public RefCountedThreadSafe<Connection> {
// marked failed. The caller is expected to check if 'call' has been cancelled
// before making the call.
// Takes ownership of the 'call' object regardless of whether it succeeds or fails.
- void QueueOutboundCall(const std::shared_ptr<OutboundCall> &call);
+ void QueueOutboundCall(std::shared_ptr<OutboundCall> call);
// Queue a call response back to the client on the server side.
//
http://git-wip-us.apache.org/repos/asf/kudu/blob/df65df65/src/kudu/rpc/reactor.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/reactor.cc b/src/kudu/rpc/reactor.cc
index 3898196..f7ff78b 100644
--- a/src/kudu/rpc/reactor.cc
+++ b/src/kudu/rpc/reactor.cc
@@ -323,7 +323,7 @@ void ReactorThread::RegisterConnection(scoped_refptr<Connection> conn) {
server_conns_.emplace_back(std::move(conn));
}
-void ReactorThread::AssignOutboundCall(const shared_ptr<OutboundCall>& call) {
+void ReactorThread::AssignOutboundCall(shared_ptr<OutboundCall> call) {
DCHECK(IsCurrentThread());
// Skip if the outbound has been cancelled already.
@@ -340,7 +340,7 @@ void ReactorThread::AssignOutboundCall(const shared_ptr<OutboundCall>& call) {
return;
}
- conn->QueueOutboundCall(call);
+ conn->QueueOutboundCall(std::move(call));
}
void ReactorThread::CancelOutboundCall(const shared_ptr<OutboundCall>& call) {
@@ -835,7 +835,7 @@ class AssignOutboundCallTask : public ReactorTask {
: call_(std::move(call)) {}
void Run(ReactorThread* reactor) override {
- reactor->AssignOutboundCall(call_);
+ reactor->AssignOutboundCall(std::move(call_));
delete this;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/df65df65/src/kudu/rpc/reactor.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/reactor.h b/src/kudu/rpc/reactor.h
index 370c4b7..cef966b 100644
--- a/src/kudu/rpc/reactor.h
+++ b/src/kudu/rpc/reactor.h
@@ -256,7 +256,7 @@ class ReactorThread {
// Assign a new outbound call to the appropriate connection object.
// If this fails, the call is marked failed and completed.
- void AssignOutboundCall(const std::shared_ptr<OutboundCall> &call);
+ void AssignOutboundCall(std::shared_ptr<OutboundCall> call);
// Cancel the outbound call. May update corresponding connection
// object to remove call from the CallAwaitingResponse object.
[2/2] kudu git commit: Fix bug causing undercounting of thread count
metric
Posted by da...@apache.org.
Fix bug causing undercounting of thread count metric
Early in tserver startup a handful (~5) threads are created before
StartThreadInstrumentation is called, which causes the thread manager
not to count them in the thread count metrics.
I found this bug because it would cause the thread count metric to dip
below zero on tserver shutdown, since the uncounted threads would still
decrement the metric. The fix is straightfoward and simplifies the code:
threads should always update the thread count metrics, even if the
thread pool is not yet registered with the metric handler.
Change-Id: Icd36b5c7d0ed1e157c0960011e0d8e44e143c205
Reviewed-on: http://gerrit.cloudera.org:8080/9049
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/d3165ed2
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/d3165ed2
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/d3165ed2
Branch: refs/heads/master
Commit: d3165ed2b511d40a34c9f27a41bfe041b9c45bdc
Parents: df65df6
Author: Dan Burkert <da...@danburkert.com>
Authored: Wed Jan 17 18:07:41 2018 -0800
Committer: Dan Burkert <da...@apache.org>
Committed: Thu Jan 18 02:50:15 2018 +0000
----------------------------------------------------------------------
src/kudu/util/thread.cc | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/d3165ed2/src/kudu/util/thread.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/thread.cc b/src/kudu/util/thread.cc
index 0378bb6..b5db351 100644
--- a/src/kudu/util/thread.cc
+++ b/src/kudu/util/thread.cc
@@ -158,8 +158,7 @@ static GoogleOnceType once = GOOGLE_ONCE_INIT;
class ThreadMgr {
public:
ThreadMgr()
- : metrics_enabled_(false),
- threads_started_metric_(0),
+ : threads_started_metric_(0),
threads_running_metric_(0) {
}
@@ -211,15 +210,12 @@ class ThreadMgr {
// All thread categorys, keyed on the category name.
typedef map<string, ThreadCategory> ThreadCategoryMap;
- // Protects thread_categories_ and metrics_enabled_
+ // Protects thread_categories_ and thread metrics.
Mutex lock_;
// All thread categorys that ever contained a thread, even if empty
ThreadCategoryMap thread_categories_;
- // True after StartInstrumentation(..) returns
- bool metrics_enabled_;
-
// Counters to track all-time total number of threads, and the
// current number of running threads.
uint64_t threads_started_metric_;
@@ -263,7 +259,6 @@ void ThreadMgr::SetThreadName(const string& name, int64_t tid) {
Status ThreadMgr::StartInstrumentation(const scoped_refptr<MetricEntity>& metrics,
WebCallbackRegistry* web) {
MutexLock l(lock_);
- metrics_enabled_ = true;
// Use function gauges here so that we can register a unique copy of these metrics in
// multiple tservers, even though the ThreadMgr is itself a singleton.
@@ -325,10 +320,8 @@ void ThreadMgr::AddThread(const pthread_t& pthread_id, const string& name,
{
MutexLock l(lock_);
thread_categories_[category][pthread_id] = ThreadDescriptor(category, name, tid);
- if (metrics_enabled_) {
- threads_running_metric_++;
- threads_started_metric_++;
- }
+ threads_running_metric_++;
+ threads_started_metric_++;
}
ANNOTATE_IGNORE_SYNC_END();
ANNOTATE_IGNORE_READS_AND_WRITES_END();
@@ -342,9 +335,7 @@ void ThreadMgr::RemoveThread(const pthread_t& pthread_id, const string& category
auto category_it = thread_categories_.find(category);
DCHECK(category_it != thread_categories_.end());
category_it->second.erase(pthread_id);
- if (metrics_enabled_) {
- threads_running_metric_--;
- }
+ threads_running_metric_--;
}
ANNOTATE_IGNORE_SYNC_END();
ANNOTATE_IGNORE_READS_AND_WRITES_END();
@@ -403,9 +394,7 @@ void ThreadMgr::ThreadPathHandler(const WebCallbackRegistry::WebRequest& req,
(*output) << "</tbody></table>";
} else {
(*output) << "<h2>Thread Groups</h2>";
- if (metrics_enabled_) {
- (*output) << "<h4>" << threads_running_metric_ << " thread(s) running";
- }
+ (*output) << "<h4>" << threads_running_metric_ << " thread(s) running";
(*output) << "<a href='/threadz?group=all'><h3>All Threads</h3>";
for (const ThreadCategoryMap::value_type& category : thread_categories_) {