You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2019/03/20 03:44:25 UTC
[kudu] branch branch-1.9.x updated (6ddc337 -> 4e31b96)
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a change to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from 6ddc337 [docker] Fix build to work outside of Git WD
new 24341a7 [tests] fix running tests under the super-user
new ddfcb87 [master] introduce cache for location mapping assignments
new b267c9f [master] use cache for assigned locations
new 3793896 [master] log on longer location mapping command runs
new 01dd2be KUDU-2743 [subprocess] test to repro deadlock in Subproces::Run()
new 4e31b96 KUDU-2743 [subprocess] use RAW_LOG() in child process
The 6 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/kudu/client/client-test.cc | 44 ++++-
.../integration-tests/location_assignment-itest.cc | 70 ++++++++
src/kudu/master/CMakeLists.txt | 2 +
src/kudu/master/location_cache-test.cc | 179 +++++++++++++++++++++
src/kudu/master/location_cache.cc | 154 ++++++++++++++++++
src/kudu/master/location_cache.h | 88 ++++++++++
src/kudu/master/master-test.cc | 25 ++-
src/kudu/master/master.cc | 9 +-
src/kudu/master/master.h | 11 +-
src/kudu/master/master_service.cc | 10 +-
src/kudu/master/ts_descriptor-test.cc | 24 +--
src/kudu/master/ts_descriptor.cc | 72 +--------
src/kudu/master/ts_descriptor.h | 16 +-
src/kudu/master/ts_manager.cc | 9 +-
src/kudu/master/ts_manager.h | 10 +-
src/kudu/rpc/negotiation-test.cc | 9 +-
src/kudu/tools/ksck_remote-test.cc | 36 ++---
src/kudu/util/env-test.cc | 17 +-
src/kudu/util/signal.cc | 28 +++-
src/kudu/util/subprocess-test.cc | 48 ++++++
src/kudu/util/subprocess.cc | 66 ++++++--
21 files changed, 775 insertions(+), 152 deletions(-)
create mode 100644 src/kudu/master/location_cache-test.cc
create mode 100644 src/kudu/master/location_cache.cc
create mode 100644 src/kudu/master/location_cache.h
[kudu] 05/06: KUDU-2743 [subprocess] test to repro deadlock in
Subproces::Run()
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 01dd2bec4b5c076d7d58d514ae846378994e712a
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Mar 12 20:47:27 2019 -0700
KUDU-2743 [subprocess] test to repro deadlock in Subproces::Run()
Added a test to reproduce deadlock when fork()-ed but not yet
exec()-ed subprocess tries to log on a failed execve() call.
The test is disabled because it fails. The follow-up
changelist will fix the problem and re-enable the test.
Change-Id: Ie88073bb7fcccf24b4451fe3c4e4cd8bd471ca67
Reviewed-on: http://gerrit.cloudera.org:8080/12740
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
(cherry picked from commit 33479893bd6037c463e7cb89b08e1e016565e2c3)
(cherry picked from commit 4bec64a8e0c22924d1addce8d19942147b191391)
Reviewed-on: http://gerrit.cloudera.org:8080/12786
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/util/subprocess-test.cc | 46 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/src/kudu/util/subprocess-test.cc b/src/kudu/util/subprocess-test.cc
index 24d7cb3..a9b7aac 100644
--- a/src/kudu/util/subprocess-test.cc
+++ b/src/kudu/util/subprocess-test.cc
@@ -37,6 +37,7 @@
#include "kudu/gutil/macros.h"
#include "kudu/gutil/strings/substitute.h"
+#include "kudu/util/barrier.h"
#include "kudu/util/env.h"
#include "kudu/util/monotime.h"
#include "kudu/util/path_util.h"
@@ -352,6 +353,51 @@ TEST_F(SubprocessTest, TestSubprocessInterruptionHandling) {
}
}
+TEST_F(SubprocessTest, DISABLED_TestSubprocessDeadlockOnLogging) {
+ int kNumLoggingThreads = 8;
+ // Participants are the logging threads and the main test thread.
+ Barrier barrier(kNumLoggingThreads + 1);
+
+ // It should not take too long to run the scenario unless it's deadlocked.
+ alarm(15);
+ SCOPED_CLEANUP({
+ alarm(0);
+ });
+
+ // Adding multiple threads writing logs. Each thread sleeps a bit after each
+ // message to get off from CPU.
+ atomic<bool> stop_logging(false);
+ vector<thread> threads;
+ for (auto i = 0; i < kNumLoggingThreads; ++i) {
+ const auto thread_idx = i;
+ threads.emplace_back([&barrier, &stop_logging, thread_idx] () {
+ barrier.Wait();
+ size_t msg_id = 0;
+ while (!stop_logging) {
+ LOG(INFO) << Substitute("$0: $1", thread_idx, msg_id++);
+ SleepFor(MonoDelta::FromNanoseconds(thread_idx));
+ }
+ });
+ }
+
+ SCOPED_CLEANUP({
+ stop_logging = true;
+ for (auto& thread : threads) {
+ thread.join();
+ }
+ });
+
+ barrier.Wait();
+ // Prior to the patch that fixed the problem, it was enough to have just one
+ // iteration to have the spawned process deadlocked. However, it does not hurt
+ // to run several iterations to give the issue, if any, a greater chance to
+ // manifest itself.
+ for (auto i = 0; i < 32; ++i) {
+ auto s = Subprocess::Call("./nonexistent.file");
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ }
+}
+
#ifdef __linux__
// This test requires a system with /proc/<pid>/stat.
TEST_F(SubprocessTest, TestGetProcfsState) {
[kudu] 06/06: KUDU-2743 [subprocess] use RAW_LOG() in child process
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 4e31b964c99b584dfe35bb6b416c481ce8280501
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Mar 12 19:51:13 2019 -0700
KUDU-2743 [subprocess] use RAW_LOG() in child process
For a newly fork()-ed, but not yet exec()-ed process, only async-signal
safe functions should be called. At least, glog's LOG() should not be
called since it involves acquiring a lock while flushing the message
into the log sink. Otherwise, the child process can deadlock on attempt
to write a log message if glog's mutex has been copied from the parent's
address space in a locked state.
Change-Id: Ic9dca4ca8b1a6d72c9fc818ea41109c80ace3e39
Reviewed-on: http://gerrit.cloudera.org:8080/12739
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
(cherry picked from commit d56930fbcdceff911efc8ec30b5c769901888d3d)
(cherry picked from commit 3270d67669e5cf0038fad655178c8c28082425d4)
Reviewed-on: http://gerrit.cloudera.org:8080/12787
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/util/signal.cc | 28 ++++++++++++++---
src/kudu/util/subprocess-test.cc | 4 ++-
src/kudu/util/subprocess.cc | 66 +++++++++++++++++++++++++++++++---------
3 files changed, 78 insertions(+), 20 deletions(-)
diff --git a/src/kudu/util/signal.cc b/src/kudu/util/signal.cc
index e8b6e79..37e78f6 100644
--- a/src/kudu/util/signal.cc
+++ b/src/kudu/util/signal.cc
@@ -17,16 +17,28 @@
#include "kudu/util/signal.h"
-#include <glog/logging.h>
+#include <cerrno>
+
+#include <glog/logging.h> // IWYU pragma: keep
+#include <glog/raw_logging.h>
namespace kudu {
+// Using RAW_LOG() and logging directly into stderr instead of using more
+// advanced LOG() to keep all these function as async-signal-safe as possible.
+//
+// NOTE: RAW_LOG() uses vsnprintf(), and it's not async-signal-safe
+// strictly speaking (in some cases may call malloc(), etc.)
+
void SetSignalHandler(int signal, SignalHandlerCallback handler) {
struct sigaction act;
act.sa_handler = handler;
sigemptyset(&act.sa_mask);
act.sa_flags = 0;
- PCHECK(sigaction(signal, &act, nullptr) == 0);
+ if (sigaction(signal, &act, nullptr) != 0) {
+ int err = errno;
+ RAW_LOG(FATAL, "sigaction() failed: [%d]", err);
+ }
}
void IgnoreSigPipe() {
@@ -39,9 +51,15 @@ void ResetSigPipeHandlerToDefault() {
// We unblock all signal masks since they are inherited.
void ResetAllSignalMasksToUnblocked() {
- sigset_t signals;
- PCHECK(sigfillset(&signals) == 0);
- PCHECK(sigprocmask(SIG_UNBLOCK, &signals, nullptr) == 0);
+ sigset_t sigset;
+ if (sigfillset(&sigset) != 0) {
+ int err = errno;
+ RAW_LOG(FATAL, "sigfillset() failed: [%d]", err);
+ }
+ if (sigprocmask(SIG_UNBLOCK, &sigset, nullptr) != 0) {
+ int err = errno;
+ RAW_LOG(FATAL, "sigprocmask() failed: [%d]", err);
+ }
}
} // namespace kudu
diff --git a/src/kudu/util/subprocess-test.cc b/src/kudu/util/subprocess-test.cc
index a9b7aac..99c2994 100644
--- a/src/kudu/util/subprocess-test.cc
+++ b/src/kudu/util/subprocess-test.cc
@@ -353,7 +353,9 @@ TEST_F(SubprocessTest, TestSubprocessInterruptionHandling) {
}
}
-TEST_F(SubprocessTest, DISABLED_TestSubprocessDeadlockOnLogging) {
+// Test to make sure the spawned child process does not deadlock while running
+// the code between fork() and exec().
+TEST_F(SubprocessTest, TestSubprocessDeadlockOnLogging) {
int kNumLoggingThreads = 8;
// Participants are the logging threads and the main test thread.
Barrier barrier(kNumLoggingThreads + 1);
diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc
index d68cb7f..f992ba4 100644
--- a/src/kudu/util/subprocess.cc
+++ b/src/kudu/util/subprocess.cc
@@ -39,6 +39,7 @@
#include <ev++.h>
#include <glog/logging.h>
+#include <glog/raw_logging.h>
#include <glog/stl_logging.h>
#include "kudu/gutil/basictypes.h"
@@ -126,7 +127,7 @@ void CloseNonStandardFDs(DIR* fd_dir) {
// make it as lean and mean as possible -- this runs in the subprocess
// after a fork, so there's some possibility that various global locks
// inside malloc() might be held, so allocating memory is a no-no.
- PCHECK(fd_dir != nullptr);
+ RAW_CHECK(fd_dir != nullptr, "fd_dir is null");
int dir_fd = dirfd(fd_dir);
struct DIRENT* ent;
@@ -165,12 +166,16 @@ void RedirectToDevNull(int fd) {
// file descriptors later on.
int dev_null;
RETRY_ON_EINTR(dev_null, open("/dev/null", O_WRONLY));
- if (dev_null < 0) {
- PLOG(WARNING) << "failed to open /dev/null";
+ if (dev_null == -1) {
+ int err = errno;
+ RAW_LOG(WARNING, "failed to open /dev/null: [%d]", err);
} else {
int ret;
RETRY_ON_EINTR(ret, dup2(dev_null, fd));
- PCHECK(ret);
+ if (ret == -1) {
+ int err = errno;
+ RAW_LOG(FATAL, "dup2() on /dev/null failed: [%d]", err);
+ }
}
}
@@ -369,6 +374,23 @@ Status Subprocess::Start() {
return Status::RuntimeError("Unable to fork", ErrnoToString(errno), errno);
}
if (ret == 0) { // We are the child
+ // As a general note, it's not safe to call non-async-signal-safe functions
+ // in the child process between fork() and exec(). Surprisingly, a call to
+ // LOG() locks a mutex that may have been copied from the parent's address
+ // space in an already locked state, so it is not async-signal-safe and
+ // can deadlock the child if called. So, in this vulnerable state the child
+ // outputs log messages using RAW_LOG() instead directly into stderr.
+ // RAW_LOG() uses vsnprintf() under the hood: it's not async-signal-safe
+ // strictly speaking (might call malloc() and getenv() in some cases which
+ // might acquire locks themselves), but it's much better than using LOG()
+ // where it can simply deadlock on glog's mutex. BTW, some allocators like
+ // tcmalloc install pthread_atfork() handlers, so with tcmalloc we have
+ // more safety with vsnprintf().
+ //
+ // An alternative approach might be to use some additional functionality
+ // in glog library (once implemented) to establish thread_atfork() handlers;
+ // see https://github.com/robi56/google-glog/issues/101 for details.
+
// Send the child a SIGTERM when the parent dies. This is done as early
// as possible in the child's life to prevent any orphaning whatsoever
// (e.g. from KUDU-402).
@@ -382,9 +404,13 @@ Status Subprocess::Start() {
if (fd_state_[STDIN_FILENO] == PIPED) {
int dup2_ret;
RETRY_ON_EINTR(dup2_ret, dup2(child_stdin[0], STDIN_FILENO));
- PCHECK(dup2_ret == STDIN_FILENO);
+ if (dup2_ret != STDIN_FILENO) {
+ int err = errno;
+ RAW_LOG(FATAL, "dup2() failed (STDIN): [%d]", err);
+ }
} else {
- DCHECK_EQ(SHARED, fd_state_[STDIN_FILENO]);
+ RAW_DCHECK(SHARED == fd_state_[STDIN_FILENO],
+ "unexpected state of STDIN");
}
// stdout
@@ -392,7 +418,10 @@ Status Subprocess::Start() {
case PIPED: {
int dup2_ret;
RETRY_ON_EINTR(dup2_ret, dup2(child_stdout[1], STDOUT_FILENO));
- PCHECK(dup2_ret == STDOUT_FILENO);
+ if (dup2_ret != STDOUT_FILENO) {
+ int err = errno;
+ RAW_LOG(FATAL, "dup2() failed (STDOUT): [%d]", err);
+ }
break;
}
case DISABLED: {
@@ -400,7 +429,8 @@ Status Subprocess::Start() {
break;
}
default:
- DCHECK_EQ(SHARED, fd_state_[STDOUT_FILENO]);
+ RAW_DCHECK(SHARED == fd_state_[STDOUT_FILENO],
+ "unexpected state of STDOUT");
break;
}
@@ -409,7 +439,10 @@ Status Subprocess::Start() {
case PIPED: {
int dup2_ret;
RETRY_ON_EINTR(dup2_ret, dup2(child_stderr[1], STDERR_FILENO));
- PCHECK(dup2_ret == STDERR_FILENO);
+ if (dup2_ret != STDERR_FILENO) {
+ int err = errno;
+ RAW_LOG(FATAL, "dup2() failed (STDERR): [%d]", err);
+ }
break;
}
case DISABLED: {
@@ -417,7 +450,8 @@ Status Subprocess::Start() {
break;
}
default:
- DCHECK_EQ(SHARED, fd_state_[STDERR_FILENO]);
+ RAW_DCHECK(SHARED == fd_state_[STDERR_FILENO],
+ "unexpected state of STDERR");
break;
}
@@ -425,7 +459,10 @@ Status Subprocess::Start() {
// the write side should be closed upon execvp().
int close_ret;
RETRY_ON_EINTR(close_ret, close(sync_pipe[0]));
- PCHECK(close_ret == 0);
+ if (close_ret == -1) {
+ int err = errno;
+ RAW_LOG(FATAL, "close() on the read side of sync pipe failed: [%d]", err);
+ }
CloseNonStandardFDs(fd_dir);
@@ -438,8 +475,9 @@ Status Subprocess::Start() {
ResetSigPipeHandlerToDefault();
// Set the current working directory of the subprocess.
- if (!cwd_.empty()) {
- PCHECK(chdir(cwd_.c_str()) == 0);
+ if (!cwd_.empty() && chdir(cwd_.c_str()) == -1) {
+ int err = errno;
+ RAW_LOG(FATAL, "chdir() to '%s' failed: [%d]", cwd_.c_str(), err);
}
// Set the environment for the subprocess. This is more portable than
@@ -452,7 +490,7 @@ Status Subprocess::Start() {
execvp(program_.c_str(), &argv_ptrs[0]);
int err = errno;
- PLOG(ERROR) << "Couldn't exec " << program_;
+ RAW_LOG(ERROR, "could not exec '%s': [%d]", program_.c_str(), err);
_exit(err);
} else {
// We are the parent
[kudu] 02/06: [master] introduce cache for location mapping
assignments
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit ddfcb873eb251e87049f0e5b3aabb417bd27c7e7
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Wed Feb 27 21:53:43 2019 -0800
[master] introduce cache for location mapping assignments
This changelist adds a very primitive cache for location assignments.
The cache does not prevent running multiple commands for the same
location key if an entry is not present in the cache.
A unit test is also added.
Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Reviewed-on: http://gerrit.cloudera.org:8080/12634
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Kudu Jenkins
(cherry picked from commit ae6bbcaabd20955119f1d945d276589538dae928)
Reviewed-on: http://gerrit.cloudera.org:8080/12783
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/master/CMakeLists.txt | 2 +
src/kudu/master/location_cache-test.cc | 179 +++++++++++++++++++++++++++++++++
src/kudu/master/location_cache.cc | 152 ++++++++++++++++++++++++++++
src/kudu/master/location_cache.h | 88 ++++++++++++++++
4 files changed, 421 insertions(+)
diff --git a/src/kudu/master/CMakeLists.txt b/src/kudu/master/CMakeLists.txt
index 89fd9c6..79f38ce 100644
--- a/src/kudu/master/CMakeLists.txt
+++ b/src/kudu/master/CMakeLists.txt
@@ -35,6 +35,7 @@ ADD_EXPORTABLE_LIBRARY(master_proto
set(MASTER_SRCS
catalog_manager.cc
hms_notification_log_listener.cc
+ location_cache.cc
master.cc
master_cert_authority.cc
master_options.cc
@@ -79,6 +80,7 @@ SET_KUDU_TEST_LINK_LIBS(
ADD_KUDU_TEST(catalog_manager-test)
ADD_KUDU_TEST(hms_notification_log_listener-test)
+ADD_KUDU_TEST(location_cache-test DATA_FILES ../scripts/first_argument.sh)
ADD_KUDU_TEST(master-test RESOURCE_LOCK "master-web-port"
DATA_FILES ../scripts/first_argument.sh)
ADD_KUDU_TEST(mini_master-test RESOURCE_LOCK "master-web-port")
diff --git a/src/kudu/master/location_cache-test.cc b/src/kudu/master/location_cache-test.cc
new file mode 100644
index 0000000..96c1bea
--- /dev/null
+++ b/src/kudu/master/location_cache-test.cc
@@ -0,0 +1,179 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/master/location_cache.h"
+
+#include <cstdint>
+#include <string>
+#include <thread>
+#include <vector>
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include "kudu/gutil/ref_counted.h"
+#include "kudu/gutil/strings/substitute.h"
+#include "kudu/util/metrics.h"
+#include "kudu/util/path_util.h"
+#include "kudu/util/status.h"
+#include "kudu/util/test_macros.h"
+#include "kudu/util/test_util.h"
+
+METRIC_DECLARE_counter(location_mapping_cache_hits);
+METRIC_DECLARE_counter(location_mapping_cache_queries);
+
+using std::string;
+using std::thread;
+using std::vector;
+using strings::Substitute;
+
+namespace kudu {
+namespace master {
+
+// Targeted test for LocationCache.
+class LocationCacheTest : public KuduTest {
+ protected:
+ void SetUp() override {
+ KuduTest::SetUp();
+ metric_entity_ = METRIC_ENTITY_server.Instantiate(&metric_registry_,
+ "LocationCacheTest");
+ }
+
+ void CheckMetrics(int64_t expected_queries, int64_t expected_hits) {
+ scoped_refptr<Counter> cache_queries(metric_entity_->FindOrCreateCounter(
+ &METRIC_location_mapping_cache_queries));
+ ASSERT_NE(nullptr, cache_queries.get());
+ ASSERT_EQ(expected_queries, cache_queries->value());
+
+ scoped_refptr<Counter> cache_hits(metric_entity_->FindOrCreateCounter(
+ &METRIC_location_mapping_cache_hits));
+ ASSERT_NE(nullptr, cache_hits.get());
+ ASSERT_EQ(expected_hits, cache_hits->value());
+ }
+
+ MetricRegistry metric_registry_;
+ scoped_refptr<MetricEntity> metric_entity_;
+};
+
+TEST_F(LocationCacheTest, EmptyMappingCommand) {
+ LocationCache cache(" ", metric_entity_.get());
+ string location;
+ auto s = cache.GetLocation("na", &location);
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "invalid empty location mapping command");
+ NO_FATALS(CheckMetrics(1, 0));
+}
+
+TEST_F(LocationCacheTest, MappingCommandFailureExitStatus) {
+ LocationCache cache("/sbin/nologin", metric_entity_.get());
+ string location;
+ auto s = cache.GetLocation("na", &location);
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "failed to run location mapping command: ");
+ NO_FATALS(CheckMetrics(1, 0));
+}
+
+TEST_F(LocationCacheTest, MappingCommandEmptyOutput) {
+ LocationCache cache("/bin/cat", metric_entity_.get());
+ string location;
+ auto s = cache.GetLocation("/dev/null", &location);
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "location mapping command returned invalid empty location");
+ NO_FATALS(CheckMetrics(1, 0));
+}
+
+TEST_F(LocationCacheTest, MappingCommandReturnsInvalidLocation) {
+ const string cmd_path = JoinPathSegments(GetTestExecutableDirectory(),
+ "testdata/first_argument.sh");
+ const string location_mapping_cmd = Substitute("$0 invalid.location",
+ cmd_path);
+ LocationCache cache(location_mapping_cmd, metric_entity_.get());
+ string location;
+ auto s = cache.GetLocation("na", &location);
+ ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
+ ASSERT_STR_CONTAINS(
+ s.ToString(), "location mapping command returned invalid location");
+ NO_FATALS(CheckMetrics(1, 0));
+}
+
+TEST_F(LocationCacheTest, HappyPath) {
+ const string kRefLocation = "/ref_location";
+ const string cmd_path = JoinPathSegments(GetTestExecutableDirectory(),
+ "testdata/first_argument.sh");
+ const string location_mapping_cmd = Substitute("$0 $1",
+ cmd_path, kRefLocation);
+ LocationCache cache(location_mapping_cmd, metric_entity_.get());
+ NO_FATALS(CheckMetrics(0, 0));
+
+ string location;
+ auto s = cache.GetLocation("key_0", &location);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ ASSERT_EQ(kRefLocation, location);
+ NO_FATALS(CheckMetrics(1, 0));
+
+ s = cache.GetLocation("key_1", &location);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ ASSERT_EQ(kRefLocation, location);
+ NO_FATALS(CheckMetrics(2, 0));
+
+ s = cache.GetLocation("key_1", &location);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ ASSERT_EQ(kRefLocation, location);
+ NO_FATALS(CheckMetrics(3, 1));
+
+ s = cache.GetLocation("key_0", &location);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ ASSERT_EQ(kRefLocation, location);
+ NO_FATALS(CheckMetrics(4, 2));
+}
+
+TEST_F(LocationCacheTest, ConcurrentRequests) {
+ static constexpr auto kNumThreads = 32;
+ const string kRefLocation = "/ref_location";
+ const string cmd_path = JoinPathSegments(GetTestExecutableDirectory(),
+ "testdata/first_argument.sh");
+ const string location_mapping_cmd = Substitute("$0 $1",
+ cmd_path, kRefLocation);
+ LocationCache cache(location_mapping_cmd, metric_entity_.get());
+ NO_FATALS(CheckMetrics(0, 0));
+
+ for (auto iter = 0; iter < 2; ++iter) {
+ vector<thread> threads;
+ threads.reserve(kNumThreads);
+ for (auto idx = 0; idx < kNumThreads; ++idx) {
+ threads.emplace_back([&cache, &kRefLocation, idx]() {
+ string location;
+ auto s = cache.GetLocation(Substitute("key_$0", idx), &location);
+ CHECK(s.ok()) << s.ToString();
+ CHECK_EQ(kRefLocation, location);
+ });
+ }
+ for (auto& t : threads) {
+ t.join();
+ }
+ // Expecting to account for every query, and the follow-up iteration
+ // should result in every query hitting the cache.
+ NO_FATALS(CheckMetrics(kNumThreads * (iter + 1),
+ kNumThreads * iter));
+ }
+}
+
+} // namespace master
+} // namespace kudu
diff --git a/src/kudu/master/location_cache.cc b/src/kudu/master/location_cache.cc
new file mode 100644
index 0000000..b79b259
--- /dev/null
+++ b/src/kudu/master/location_cache.cc
@@ -0,0 +1,152 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/master/location_cache.h"
+
+#include <cstdio>
+#include <mutex>
+#include <string>
+#include <unordered_map>
+#include <utility>
+#include <vector>
+
+#include <glog/logging.h>
+
+#include "kudu/gutil/map-util.h"
+#include "kudu/gutil/port.h"
+#include "kudu/gutil/strings/charset.h"
+#include "kudu/gutil/strings/split.h"
+#include "kudu/gutil/strings/strip.h"
+#include "kudu/gutil/strings/substitute.h"
+#include "kudu/util/subprocess.h"
+#include "kudu/util/trace.h"
+
+METRIC_DEFINE_counter(server, location_mapping_cache_hits,
+ "Location Mapping Cache Hits",
+ kudu::MetricUnit::kCacheHits,
+ "Number of times location mapping assignment used "
+ "cached data");
+METRIC_DEFINE_counter(server, location_mapping_cache_queries,
+ "Location Mapping Cache Queries",
+ kudu::MetricUnit::kCacheQueries,
+ "Number of queries to the location mapping cache");
+
+using std::string;
+using std::vector;
+using strings::Substitute;
+
+namespace kudu {
+namespace master {
+
+namespace {
+// Returns if 'location' is a valid location string, i.e. it begins with /
+// and consists of /-separated tokens each of which contains only characters
+// from the set [a-zA-Z0-9_-.].
+bool IsValidLocation(const string& location) {
+ if (location.empty() || location[0] != '/') {
+ return false;
+ }
+ const strings::CharSet charset("abcdefghijklmnopqrstuvwxyz"
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "0123456789"
+ "_-./");
+ for (const auto c : location) {
+ if (!charset.Test(c)) {
+ return false;
+ }
+ }
+ return true;
+}
+} // anonymous namespace
+
+LocationCache::LocationCache(string location_mapping_cmd,
+ MetricEntity* metric_entity)
+ : location_mapping_cmd_(std::move(location_mapping_cmd)) {
+ if (metric_entity != nullptr) {
+ location_mapping_cache_hits_ = metric_entity->FindOrCreateCounter(
+ &METRIC_location_mapping_cache_hits);
+ location_mapping_cache_queries_ = metric_entity->FindOrCreateCounter(
+ &METRIC_location_mapping_cache_queries);
+ }
+}
+
+Status LocationCache::GetLocation(const string& key, string* location) {
+ if (PREDICT_TRUE(location_mapping_cache_queries_)) {
+ location_mapping_cache_queries_->Increment();
+ }
+ {
+ // First check whether the location for the key has already been assigned.
+ shared_lock<rw_spinlock> l(location_map_lock_);
+ const auto* value_ptr = FindOrNull(location_map_, key);
+ if (value_ptr) {
+ DCHECK(!value_ptr->empty());
+ *location = *value_ptr;
+ if (PREDICT_TRUE(location_mapping_cache_hits_)) {
+ location_mapping_cache_hits_->Increment();
+ }
+ return Status::OK();
+ }
+ }
+ string value;
+ TRACE(Substitute("key $0: assigning location", key));
+ Status s = GetLocationFromLocationMappingCmd(
+ location_mapping_cmd_, key, &value);
+ TRACE(Substitute("key $0: assigned location '$1'", key, value));
+ if (s.ok()) {
+ CHECK(!value.empty());
+ std::lock_guard<rw_spinlock> l(location_map_lock_);
+ // This simple implementation doesn't protect against multiple runs of the
+ // location-mapping command for the same key.
+ InsertIfNotPresent(&location_map_, key, value);
+ *location = value;
+ }
+ return s;
+}
+
+Status LocationCache::GetLocationFromLocationMappingCmd(const string& cmd,
+ const string& key,
+ string* location) {
+ DCHECK(location);
+ vector<string> argv = strings::Split(cmd, " ", strings::SkipEmpty());
+ if (argv.empty()) {
+ return Status::RuntimeError("invalid empty location mapping command");
+ }
+ argv.push_back(key);
+ string stderr, location_temp;
+ Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
+ if (!s.ok()) {
+ return Status::RuntimeError(
+ Substitute("failed to run location mapping command: $0", s.ToString()),
+ stderr);
+ }
+ StripWhiteSpace(&location_temp);
+ // Special case an empty location for a better error.
+ if (location_temp.empty()) {
+ return Status::RuntimeError(
+ "location mapping command returned invalid empty location");
+ }
+ if (!IsValidLocation(location_temp)) {
+ return Status::RuntimeError(
+ "location mapping command returned invalid location",
+ location_temp);
+ }
+ *location = std::move(location_temp);
+ return Status::OK();
+}
+
+} // namespace master
+} // namespace kudu
diff --git a/src/kudu/master/location_cache.h b/src/kudu/master/location_cache.h
new file mode 100644
index 0000000..b89f2d9
--- /dev/null
+++ b/src/kudu/master/location_cache.h
@@ -0,0 +1,88 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#pragma once
+
+#include <string>
+#include <unordered_map>
+
+#include "kudu/gutil/macros.h"
+#include "kudu/gutil/ref_counted.h"
+#include "kudu/util/locks.h"
+#include "kudu/util/metrics.h"
+#include "kudu/util/status.h"
+
+namespace kudu {
+namespace master {
+
+// A primitive cache of unlimited capacity to store assigned locations for
+// a key. The cache entries are kept in the cache for the lifetime of the cache
+// itself.
+class LocationCache {
+ public:
+ // The location assignment command is specified by the 'location_mapping_cmd'
+ // parameter (the command might be a script or an executable). The
+ // 'metric_entity' is used to register standard cache counters: total number
+ // of queries and number of cache hits during the cache's lifetime.
+ explicit LocationCache(std::string location_mapping_cmd,
+ MetricEntity* metric_entity = nullptr);
+ ~LocationCache() = default;
+
+ // Get the location for the specified key. The key is treated as an opaque
+ // identifier.
+ //
+ // If no cached location is found, the location mapping command is run,
+ // caching the result for the lifetime of the cache.
+ //
+ // This method returns an error if there was an issue running the location
+ // assignment command.
+ Status GetLocation(const std::string& key, std::string* location);
+
+ private:
+ // Resolves an opaque 'key' into a location using the command 'cmd'.
+ // The result will be stored in 'location', which must not be null. If there
+ // is an error running the command or the output is invalid, an error Status
+ // will be returned.
+ //
+ // TODO(wdberkeley): Eventually we may want to get multiple locations at once
+ // by giving the location mapping command multiple arguments (like Hadoop).
+ static Status GetLocationFromLocationMappingCmd(const std::string& cmd,
+ const std::string& key,
+ std::string* location);
+
+ // The executable to run when assigning locations for keys which are not yet
+ // in the cache.
+ const std::string location_mapping_cmd_;
+
+ // Counter to track cache hits, i.e. when it was not necessary to run
+ // the location assignment command.
+ scoped_refptr<Counter> location_mapping_cache_hits_;
+
+ // Counter to track overall cache queries, i.e. hits plus misses. Every miss
+ // results in the location assignment command being run.
+ scoped_refptr<Counter> location_mapping_cache_queries_;
+
+ // Spinlock to protect the location assignment map (location_map_).
+ rw_spinlock location_map_lock_;
+
+ // The location assignment map: dictionary of key --> location.
+ std::unordered_map<std::string, std::string> location_map_;
+
+ DISALLOW_COPY_AND_ASSIGN(LocationCache);
+};
+
+} // namespace master
+} // namespace kudu
[kudu] 01/06: [tests] fix running tests under the super-user
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 24341a7d2089921355aacb0eb0d8f2f4dc2a4e1b
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Thu Feb 14 12:17:43 2019 -0800
[tests] fix running tests under the super-user
Recently I found myself running Kudu tests as a super-user on a VM.
A couple of test failed, and this patch fixes that.
This is a test-only changelist, it doesn't change any functionality.
Change-Id: I05bf220ee8000209a3e36faa9f21fd43ab8bdc9f
Reviewed-on: http://gerrit.cloudera.org:8080/12486
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
(cherry picked from commit 8f6d2f9a492d9ab3e92e6e24d2b9810ab108379a)
Reviewed-on: http://gerrit.cloudera.org:8080/12782
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/rpc/negotiation-test.cc | 9 +++++++--
src/kudu/util/env-test.cc | 17 +++++++++++++----
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/kudu/rpc/negotiation-test.cc b/src/kudu/rpc/negotiation-test.cc
index f3990ac..bebc5e9 100644
--- a/src/kudu/rpc/negotiation-test.cc
+++ b/src/kudu/rpc/negotiation-test.cc
@@ -1166,10 +1166,15 @@ TEST_F(TestNegotiation, TestPreflight) {
// Try with an inaccessible keytab.
CHECK_ERR(chmod(kt_path.c_str(), 0000));
s = ServerNegotiation::PreflightCheckGSSAPI("kudu");
- ASSERT_FALSE(s.ok());
+ if (geteuid() == 0) {
+ // The super-user can acess the 'inaccessible' keytab file anyway.
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ } else {
+ ASSERT_FALSE(s.ok()) << s.ToString();
#ifndef KRB5_VERSION_LE_1_10
- ASSERT_STR_MATCHES(s.ToString(), "error accessing keytab: Permission denied");
+ ASSERT_STR_MATCHES(s.ToString(), "error accessing keytab: Permission denied");
#endif
+ }
CHECK_ERR(unlink(kt_path.c_str()));
// Try with a keytab that has the wrong credentials.
diff --git a/src/kudu/util/env-test.cc b/src/kudu/util/env-test.cc
index 1c7f899..5bc1f0b 100644
--- a/src/kudu/util/env-test.cc
+++ b/src/kudu/util/env-test.cc
@@ -771,10 +771,15 @@ TEST_F(TestEnv, TestWalkBadPermissions) {
PCHECK(chmod(kTestPath.c_str(), stat_buf.st_mode) == 0);
});
- // A walk on a directory without execute permission should fail.
+ // A walk on a directory without execute permission should fail,
+ // unless the calling process has super-user's effective ID.
Status s = env_->Walk(kTestPath, Env::PRE_ORDER, Bind(&NoopTestWalkCb));
- ASSERT_TRUE(s.IsIOError());
- ASSERT_STR_CONTAINS(s.ToString(), "One or more errors occurred");
+ if (geteuid() == 0) {
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ } else {
+ ASSERT_TRUE(s.IsIOError()) << s.ToString();
+ ASSERT_STR_CONTAINS(s.ToString(), "One or more errors occurred");
+ }
}
static Status TestWalkErrorCb(int* num_calls,
@@ -836,7 +841,11 @@ TEST_F(TestEnv, TestGlobPermissionDenied) {
});
vector<string> matches;
Status s = env_->Glob(JoinPathSegments(dir, "*"), &matches);
- ASSERT_STR_MATCHES(s.ToString(), "IO error: glob failed for /.*: Permission denied");
+ if (geteuid() == 0) {
+ ASSERT_TRUE(s.ok()) << s.ToString();
+ } else {
+ ASSERT_STR_MATCHES(s.ToString(), "IO error: glob failed for /.*: Permission denied");
+ }
}
TEST_F(TestEnv, TestGetBlockSize) {
[kudu] 04/06: [master] log on longer location mapping command runs
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 37938969158d9358b3fa580bbb75494415abd34d
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Fri Mar 1 13:18:05 2019 -0800
[master] log on longer location mapping command runs
This patch adds INFO logging when it takes more than 1 second
to run the location mapping command.
Change-Id: Icc420c704b1fad8bddd54285f1a9aa12b71095f0
Reviewed-on: http://gerrit.cloudera.org:8080/12644
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Kudu Jenkins
(cherry picked from commit 4d4552d06934f14ed608cf6eb330428df6e8368d)
Reviewed-on: http://gerrit.cloudera.org:8080/12785
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/master/location_cache.cc | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/kudu/master/location_cache.cc b/src/kudu/master/location_cache.cc
index b79b259..f4f5378 100644
--- a/src/kudu/master/location_cache.cc
+++ b/src/kudu/master/location_cache.cc
@@ -32,6 +32,7 @@
#include "kudu/gutil/strings/split.h"
#include "kudu/gutil/strings/strip.h"
#include "kudu/gutil/strings/substitute.h"
+#include "kudu/util/stopwatch.h"
#include "kudu/util/subprocess.h"
#include "kudu/util/trace.h"
@@ -127,6 +128,7 @@ Status LocationCache::GetLocationFromLocationMappingCmd(const string& cmd,
}
argv.push_back(key);
string stderr, location_temp;
+ SCOPED_LOG_SLOW_EXECUTION(WARNING, 1000, "running location mapping command");
Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
if (!s.ok()) {
return Status::RuntimeError(
[kudu] 03/06: [master] use cache for assigned locations
Posted by al...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.9.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit b267c9f45fa95c716ec9e525d1f4eeadbd6dd7ed
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Tue Feb 26 18:14:30 2019 -0800
[master] use cache for assigned locations
This changelist contains changes to incorporate LocationCache
into master. With this patch, master caches once assigned location
to tablet servers and clients.
Corresponding tests have been updated accordingly and a few additional
tests are added with this changelist.
Conflicts:
src/kudu/client/client-test.cc
src/kudu/tools/ksck_remote-test.cc
Change-Id: I12c8952c43a8ad352acd46c8006824b2ad9d1204
Reviewed-on: http://gerrit.cloudera.org:8080/12619
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
(cherry picked from commit 4ace91713ad81e72135cca7679e8a6e63b4382b5)
Reviewed-on: http://gerrit.cloudera.org:8080/12784
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
src/kudu/client/client-test.cc | 44 ++++++++++++-
.../integration-tests/location_assignment-itest.cc | 70 +++++++++++++++++++++
src/kudu/master/master-test.cc | 25 ++++++--
src/kudu/master/master.cc | 9 ++-
src/kudu/master/master.h | 11 +++-
src/kudu/master/master_service.cc | 10 +--
src/kudu/master/ts_descriptor-test.cc | 24 ++++----
src/kudu/master/ts_descriptor.cc | 72 +++-------------------
src/kudu/master/ts_descriptor.h | 16 ++---
src/kudu/master/ts_manager.cc | 9 ++-
src/kudu/master/ts_manager.h | 10 ++-
src/kudu/tools/ksck_remote-test.cc | 36 +++++------
12 files changed, 209 insertions(+), 127 deletions(-)
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 1979645..1693e1a 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -111,6 +111,7 @@
DECLARE_bool(allow_unsafe_replication_factor);
DECLARE_bool(fail_dns_resolution);
+DECLARE_bool(location_mapping_by_uuid);
DECLARE_bool(log_inject_latency);
DECLARE_bool(master_support_connect_to_master_rpc);
DECLARE_bool(rpc_trace_negotiation);
@@ -137,6 +138,8 @@ DEFINE_int32(test_scan_num_rows, 1000, "Number of rows to insert and scan");
METRIC_DECLARE_counter(block_manager_total_bytes_read);
METRIC_DECLARE_counter(rpcs_queue_overflow);
+METRIC_DECLARE_counter(location_mapping_cache_hits);
+METRIC_DECLARE_counter(location_mapping_cache_queries);
METRIC_DECLARE_histogram(handler_latency_kudu_master_MasterService_GetMasterRegistration);
METRIC_DECLARE_histogram(handler_latency_kudu_master_MasterService_GetTableLocations);
METRIC_DECLARE_histogram(handler_latency_kudu_master_MasterService_GetTabletLocations);
@@ -5796,6 +5799,10 @@ TEST_F(ClientTest, TestBlockScannerHijackingAttempts) {
}
}
+TEST_F(ClientTest, TestClientLocationNoLocationMappingCmd) {
+ ASSERT_TRUE(client_->location().empty());
+}
+
// Client test that assigns locations to clients and tablet servers.
// For now, assigns a uniform location to all clients and tablet servers.
class ClientWithLocationTest : public ClientTest {
@@ -5806,15 +5813,46 @@ class ClientWithLocationTest : public ClientTest {
const string location = "/foo";
FLAGS_location_mapping_cmd = strings::Substitute("$0 $1",
location_cmd_path, location);
+ FLAGS_location_mapping_by_uuid = true;
}
};
-TEST_F(ClientTest, TestClientLocationNoLocationMappingCmd) {
- ASSERT_TRUE(client_->location().empty());
+TEST_F(ClientWithLocationTest, TestClientLocation) {
+ ASSERT_EQ("/foo", client_->location());
}
-TEST_F(ClientWithLocationTest, TestClientLocation) {
+TEST_F(ClientWithLocationTest, LocationCacheMetricsOnClientConnectToCluster) {
ASSERT_EQ("/foo", client_->location());
+
+ auto& metric_entity = cluster_->mini_master()->master()->metric_entity();
+ scoped_refptr<Counter> counter_hits(
+ METRIC_location_mapping_cache_hits.Instantiate(metric_entity));
+ const auto hits_before = counter_hits->value();
+ ASSERT_EQ(0, hits_before);
+ scoped_refptr<Counter> counter_queries(
+ METRIC_location_mapping_cache_queries.Instantiate(metric_entity));
+ const auto queries_before = counter_queries->value();
+ // Expecting location assignment queries from all tablet servers and
+ // the client.
+ ASSERT_EQ(cluster_->num_tablet_servers() + 1, queries_before);
+
+ static constexpr int kIterNum = 10;
+ for (auto iter = 0; iter < kIterNum; ++iter) {
+ shared_ptr<KuduClient> client;
+ ASSERT_OK(KuduClientBuilder()
+ .add_master_server_addr(cluster_->mini_master()->bound_rpc_addr().ToString())
+ .Build(&client));
+ ASSERT_EQ("/foo", client->location());
+ }
+
+ // The location mapping cache should be hit every time a client is connecting
+ // from the same host as the former client. Nothing else should be touching
+ // the location assignment logic but ConnectToCluster() requests coming from
+ // the clients instantiated above.
+ const auto queries_after = counter_queries->value();
+ ASSERT_EQ(queries_before + kIterNum, queries_after);
+ const auto hits_after = counter_hits->value();
+ ASSERT_EQ(hits_before + kIterNum, hits_after);
}
} // namespace client
} // namespace kudu
diff --git a/src/kudu/integration-tests/location_assignment-itest.cc b/src/kudu/integration-tests/location_assignment-itest.cc
index cae1fa8..ce1d891 100644
--- a/src/kudu/integration-tests/location_assignment-itest.cc
+++ b/src/kudu/integration-tests/location_assignment-itest.cc
@@ -49,6 +49,8 @@
DECLARE_int32(num_replicas);
DECLARE_int32(num_tablet_servers);
+METRIC_DECLARE_counter(location_mapping_cache_hits);
+METRIC_DECLARE_counter(location_mapping_cache_queries);
METRIC_DECLARE_counter(scans_started);
METRIC_DECLARE_entity(tablet);
@@ -225,6 +227,74 @@ TEST_P(TsLocationAssignmentITest, Basic) {
NO_FATALS(cluster_->AssertNoCrashes());
}
+// Verify the behavior of the location mapping cache upon tablet server
+// registrations.
+TEST_P(TsLocationAssignmentITest, LocationMappingCacheOnTabletServerRestart) {
+ if (!AllowSlowTests()) {
+ LOG(WARNING) << "test is skipped; set KUDU_ALLOW_SLOW_TESTS=1 to run";
+ return;
+ }
+
+ NO_FATALS(StartCluster());
+ NO_FATALS(CheckLocationInfo());
+ NO_FATALS(cluster_->AssertNoCrashes());
+
+ const auto num_tablet_servers = cluster_->num_tablet_servers();
+
+ int64_t hits_before;
+ ASSERT_OK(itest::GetInt64Metric(
+ cluster_->leader_master()->bound_http_hostport(),
+ &METRIC_ENTITY_server,
+ nullptr,
+ &METRIC_location_mapping_cache_hits,
+ "value",
+ &hits_before));
+ ASSERT_EQ(0, hits_before);
+
+ int64_t queries_before;
+ ASSERT_OK(itest::GetInt64Metric(
+ cluster_->leader_master()->bound_http_hostport(),
+ &METRIC_ENTITY_server,
+ nullptr,
+ &METRIC_location_mapping_cache_queries,
+ "value",
+ &queries_before));
+ ASSERT_EQ(num_tablet_servers, queries_before);
+
+ for (auto idx = 0; idx < num_tablet_servers; ++idx) {
+ auto* ts = cluster_->tablet_server(idx);
+ ts->Shutdown();
+ ASSERT_OK(ts->Restart());
+ }
+
+ NO_FATALS(CheckLocationInfo());
+ NO_FATALS(cluster_->AssertNoCrashes());
+
+ ASSERT_EVENTUALLY([&]() {
+ int64_t hits_after;
+ ASSERT_OK(itest::GetInt64Metric(
+ cluster_->leader_master()->bound_http_hostport(),
+ &METRIC_ENTITY_server,
+ nullptr,
+ &METRIC_location_mapping_cache_hits,
+ "value",
+ &hits_after));
+ ASSERT_EQ(hits_before + num_tablet_servers, hits_after);
+ });
+
+ ASSERT_EVENTUALLY([&]() {
+ int64_t queries_after;
+ ASSERT_OK(itest::GetInt64Metric(
+ cluster_->leader_master()->bound_http_hostport(),
+ &METRIC_ENTITY_server,
+ nullptr,
+ &METRIC_location_mapping_cache_queries,
+ "value",
+ &queries_after));
+ ASSERT_EQ(queries_before + num_tablet_servers, queries_after);
+ });
+}
+
INSTANTIATE_TEST_CASE_P(, TsLocationAssignmentITest,
::testing::Combine(::testing::Values(1, 3),
::testing::Values(1, 8, 16, 32)));
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index 54dac9d..5e41fa4 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -449,6 +449,11 @@ TEST_F(MasterTest, TestRegisterAndHeartbeat) {
// Set a command that always fails.
FLAGS_location_mapping_cmd = "false";
+ // Restarting the master to take into account the new setting for the
+ // --location_mapping_cmd flag.
+ mini_master_->Shutdown();
+ ASSERT_OK(mini_master_->Restart());
+
// Set a new UUID so registration is for the first time.
auto new_common = common;
new_common.mutable_ts_instance()->set_permanent_uuid("lmc-fail-ts");
@@ -462,7 +467,7 @@ TEST_F(MasterTest, TestRegisterAndHeartbeat) {
// Registration should fail.
Status s = proxy_->TSHeartbeat(hb_req, &hb_resp, &rpc);
- ASSERT_TRUE(s.IsRemoteError());
+ ASSERT_TRUE(s.IsRemoteError()) << s.ToString();
ASSERT_STR_CONTAINS(s.ToString(), "failed to run location mapping command");
// Make sure the tablet server isn't returned to clients.
@@ -473,11 +478,7 @@ TEST_F(MasterTest, TestRegisterAndHeartbeat) {
LOG(INFO) << SecureDebugString(list_ts_resp);
ASSERT_FALSE(list_ts_resp.has_error());
- ASSERT_EQ(1, list_ts_resp.servers_size());
- ASSERT_EQ("my-ts-uuid", list_ts_resp.servers(0).instance_id().permanent_uuid());
-
- // Reset the flag.
- FLAGS_location_mapping_cmd = "";
+ ASSERT_EQ(0, list_ts_resp.servers_size());
}
}
@@ -1632,6 +1633,10 @@ TEST_F(MasterTest, TestConnectToMasterAndAssignLocation) {
const string location = "/foo";
FLAGS_location_mapping_cmd = Substitute("$0 $1", kLocationCmdPath, location);
{
+ // Restarting the master to take into account the new setting for the
+ // --location_mapping_cmd flag.
+ mini_master_->Shutdown();
+ ASSERT_OK(mini_master_->Restart());
ConnectToMasterRequestPB req;
ConnectToMasterResponsePB resp;
RpcController rpc;
@@ -1644,6 +1649,10 @@ TEST_F(MasterTest, TestConnectToMasterAndAssignLocation) {
// location should be assigned.
FLAGS_location_mapping_cmd = "false";
{
+ // Restarting the master to take into account the new setting for the
+ // --location_mapping_cmd flag.
+ mini_master_->Shutdown();
+ ASSERT_OK(mini_master_->Restart());
ConnectToMasterRequestPB req;
ConnectToMasterResponsePB resp;
RpcController rpc;
@@ -1656,6 +1665,10 @@ TEST_F(MasterTest, TestConnectToMasterAndAssignLocation) {
const string new_location = "/bar";
FLAGS_location_mapping_cmd = Substitute("$0 $1", kLocationCmdPath, new_location);
{
+ // Restarting the master to take into account the new setting for the
+ // --location_mapping_cmd flag.
+ mini_master_->Shutdown();
+ ASSERT_OK(mini_master_->Restart());
ConnectToMasterRequestPB req;
ConnectToMasterResponsePB resp;
RpcController rpc;
diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc
index ae33cd2..f6a46d2 100644
--- a/src/kudu/master/master.cc
+++ b/src/kudu/master/master.cc
@@ -37,8 +37,10 @@
#include "kudu/fs/fs_manager.h"
#include "kudu/gutil/bind.h"
#include "kudu/gutil/bind_helpers.h"
+#include "kudu/gutil/ref_counted.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/master/catalog_manager.h"
+#include "kudu/master/location_cache.h"
#include "kudu/master/master.pb.h"
#include "kudu/master/master.proxy.h"
#include "kudu/master/master_cert_authority.h"
@@ -87,6 +89,7 @@ TAG_FLAG(authz_token_validity_seconds, experimental);
DECLARE_bool(hive_metastore_sasl_enabled);
DECLARE_string(keytab_file);
+DECLARE_string(location_mapping_cmd);
using std::min;
using std::shared_ptr;
@@ -124,11 +127,15 @@ GROUP_FLAG_VALIDATOR(hive_metastore_sasl_enabled, ValidateHiveMetastoreSaslEnabl
Master::Master(const MasterOptions& opts)
: KuduServer("Master", opts, "kudu.master"),
state_(kStopped),
- ts_manager_(new TSManager(metric_entity_)),
catalog_manager_(new CatalogManager(this)),
path_handlers_(new MasterPathHandlers(this)),
opts_(opts),
registration_initialized_(false) {
+ const auto& location_cmd = FLAGS_location_mapping_cmd;
+ if (!location_cmd.empty()) {
+ location_cache_.reset(new LocationCache(location_cmd, metric_entity_.get()));
+ }
+ ts_manager_.reset(new TSManager(location_cache_.get(), metric_entity_));
}
Master::~Master() {
diff --git a/src/kudu/master/master.h b/src/kudu/master/master.h
index fda75a1..077aabd 100644
--- a/src/kudu/master/master.h
+++ b/src/kudu/master/master.h
@@ -38,6 +38,9 @@ class HostPortPB;
class MaintenanceManager;
class MonoDelta;
class ThreadPool;
+namespace master {
+class LocationCache;
+} // namespace master
namespace security {
class TokenSigner;
@@ -83,6 +86,8 @@ class Master : public kserver::KuduServer {
const MasterOptions& opts() { return opts_; }
+ LocationCache* location_cache() { return location_cache_.get(); }
+
// Get the RPC and HTTP addresses for this master instance.
Status GetMasterRegistration(ServerRegistrationPB* registration) const;
@@ -131,7 +136,6 @@ class Master : public kserver::KuduServer {
std::unique_ptr<MasterCertAuthority> cert_authority_;
std::unique_ptr<security::TokenSigner> token_signer_;
- gscoped_ptr<TSManager> ts_manager_;
gscoped_ptr<CatalogManager> catalog_manager_;
gscoped_ptr<MasterPathHandlers> path_handlers_;
@@ -151,6 +155,11 @@ class Master : public kserver::KuduServer {
// The maintenance manager for this master.
std::shared_ptr<MaintenanceManager> maintenance_manager_;
+ // A simplistic cache to track already assigned locations.
+ std::unique_ptr<LocationCache> location_cache_;
+
+ gscoped_ptr<TSManager> ts_manager_;
+
DISALLOW_COPY_AND_ASSIGN(Master);
};
diff --git a/src/kudu/master/master_service.cc b/src/kudu/master/master_service.cc
index 4a29a61..3866a3d 100644
--- a/src/kudu/master/master_service.cc
+++ b/src/kudu/master/master_service.cc
@@ -36,6 +36,7 @@
#include "kudu/gutil/strings/substitute.h"
#include "kudu/hms/hms_catalog.h"
#include "kudu/master/catalog_manager.h"
+#include "kudu/master/location_cache.h"
#include "kudu/master/master.h"
#include "kudu/master/master.pb.h"
#include "kudu/master/master_cert_authority.h"
@@ -57,7 +58,6 @@
DECLARE_bool(hive_metastore_sasl_enabled);
DECLARE_bool(raft_prepare_replacement_before_eviction);
DECLARE_string(hive_metastore_uris);
-DECLARE_string(location_mapping_cmd);
DEFINE_int32(master_inject_latency_on_tablet_lookups_ms, 0,
"Number of milliseconds that the master will sleep before responding to "
@@ -533,12 +533,12 @@ void MasterServiceImpl::ConnectToMaster(const ConnectToMasterRequestPB* /*req*/,
}
// Assign a location to the client if needed.
- if (!FLAGS_location_mapping_cmd.empty() &&
+ auto* location_cache = server_->location_cache();
+ if (location_cache != nullptr &&
PREDICT_TRUE(FLAGS_master_client_location_assignment_enabled)) {
string location;
- Status s = GetLocationFromLocationMappingCmd(FLAGS_location_mapping_cmd,
- rpc->remote_address().host(),
- &location);
+ const auto s = location_cache->GetLocation(
+ rpc->remote_address().host(), &location);
if (s.ok()) {
resp->set_client_location(location);
} else {
diff --git a/src/kudu/master/ts_descriptor-test.cc b/src/kudu/master/ts_descriptor-test.cc
index a7ec824..1593458 100644
--- a/src/kudu/master/ts_descriptor-test.cc
+++ b/src/kudu/master/ts_descriptor-test.cc
@@ -22,20 +22,18 @@
#include <vector>
#include <boost/optional/optional.hpp>
-#include <gflags/gflags_declare.h>
#include <glog/logging.h>
#include <gtest/gtest.h>
#include "kudu/common/common.pb.h"
#include "kudu/common/wire_protocol.pb.h"
#include "kudu/gutil/strings/substitute.h"
+#include "kudu/master/location_cache.h"
#include "kudu/util/path_util.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
-DECLARE_string(location_mapping_cmd);
-
using std::shared_ptr;
using std::string;
using std::vector;
@@ -75,7 +73,7 @@ TEST(TSDescriptorTest, TestRegistration) {
ServerRegistrationPB registration;
SetupBasicRegistrationInfo(uuid, &instance, ®istration);
shared_ptr<TSDescriptor> desc;
- ASSERT_OK(TSDescriptor::RegisterNew(instance, registration, &desc));
+ ASSERT_OK(TSDescriptor::RegisterNew(instance, registration, nullptr, &desc));
// Spot check some fields and the ToString value.
ASSERT_EQ(uuid, desc->permanent_uuid());
@@ -90,14 +88,15 @@ TEST(TSDescriptorTest, TestLocationCmd) {
"testdata/first_argument.sh");
// A happy case, using all allowed special characters.
const string location = "/foo-bar0/BAAZ._9-quux";
- FLAGS_location_mapping_cmd = Substitute("$0 $1", kLocationCmdPath, location);
+ const string location_cmd = Substitute("$0 $1", kLocationCmdPath, location);
+ LocationCache cache(location_cmd, nullptr);
const string uuid = "test";
NodeInstancePB instance;
ServerRegistrationPB registration;
SetupBasicRegistrationInfo(uuid, &instance, ®istration);
shared_ptr<TSDescriptor> desc;
- ASSERT_OK(TSDescriptor::RegisterNew(instance, registration, &desc));
+ ASSERT_OK(TSDescriptor::RegisterNew(instance, registration, &cache, &desc));
ASSERT_EQ(location, desc->location());
@@ -108,9 +107,10 @@ TEST(TSDescriptorTest, TestLocationCmd) {
"foo", // Doesn't begin with /.
"/foo$", // Contains the illegal character '$'.
};
- for (const auto& bad_location : bad_locations) {
- FLAGS_location_mapping_cmd = Substitute("$0 $1", kLocationCmdPath, bad_location);
- ASSERT_TRUE(desc->Register(instance, registration).IsRuntimeError());
+ for (const auto& location : bad_locations) {
+ const auto location_cmd = Substitute("$0 $1", kLocationCmdPath, location);
+ LocationCache cache(location_cmd, nullptr);
+ ASSERT_TRUE(desc->Register(instance, registration, &cache).IsRuntimeError());
}
// Bad cases where the script is invalid.
@@ -126,9 +126,9 @@ TEST(TSDescriptorTest, TestLocationCmd) {
// Command returns too many locations (i.e. contains illegal ' ' character).
Substitute("echo $0 $1", "/foo", "/bar"),
};
- for (const auto& bad_cmd : bad_cmds) {
- FLAGS_location_mapping_cmd = bad_cmd;
- ASSERT_TRUE(desc->Register(instance, registration).IsRuntimeError());
+ for (const auto& cmd : bad_cmds) {
+ LocationCache cache(cmd, nullptr);
+ ASSERT_TRUE(desc->Register(instance, registration, &cache).IsRuntimeError());
}
}
} // namespace master
diff --git a/src/kudu/master/ts_descriptor.cc b/src/kudu/master/ts_descriptor.cc
index 619f927..208adac 100644
--- a/src/kudu/master/ts_descriptor.cc
+++ b/src/kudu/master/ts_descriptor.cc
@@ -18,7 +18,6 @@
#include "kudu/master/ts_descriptor.h"
#include <cmath>
-#include <cstdio>
#include <mutex>
#include <ostream>
#include <unordered_set>
@@ -32,17 +31,15 @@
#include "kudu/common/wire_protocol.h"
#include "kudu/common/wire_protocol.pb.h"
#include "kudu/consensus/consensus.proxy.h"
-#include "kudu/gutil/strings/charset.h"
-#include "kudu/gutil/strings/split.h"
-#include "kudu/gutil/strings/strip.h"
+#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/substitute.h"
+#include "kudu/master/location_cache.h"
#include "kudu/tserver/tserver_admin.proxy.h"
#include "kudu/util/flag_tags.h"
#include "kudu/util/logging.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/net/sockaddr.h"
#include "kudu/util/pb_util.h"
-#include "kudu/util/subprocess.h"
#include "kudu/util/trace.h"
DEFINE_int32(tserver_unresponsive_timeout_ms, 60 * 1000,
@@ -75,63 +72,12 @@ using strings::Substitute;
namespace kudu {
namespace master {
-namespace {
-// Returns if 'location' is a valid location string, i.e. it begins with /
-// and consists of /-separated tokens each of which contains only characters
-// from the set [a-zA-Z0-9_-.].
-bool IsValidLocation(const string& location) {
- if (location.empty() || location[0] != '/') {
- return false;
- }
- const strings::CharSet charset("abcdefghijklmnopqrstuvwxyz"
- "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
- "0123456789"
- "_-./");
- for (const auto c : location) {
- if (!charset.Test(c)) {
- return false;
- }
- }
- return true;
-}
-} // anonymous namespace
-
-Status GetLocationFromLocationMappingCmd(const string& cmd,
- const string& host,
- string* location) {
- DCHECK(location);
- vector<string> argv = strings::Split(cmd, " ", strings::SkipEmpty());
- if (argv.empty()) {
- return Status::RuntimeError("invalid empty location mapping command");
- }
- argv.push_back(host);
- string stderr, location_temp;
- Status s = Subprocess::Call(argv, /*stdin=*/"", &location_temp, &stderr);
- if (!s.ok()) {
- return Status::RuntimeError(
- Substitute("failed to run location mapping command: $0", s.ToString()),
- stderr);
- }
- StripWhiteSpace(&location_temp);
- // Special case an empty location for a better error.
- if (location_temp.empty()) {
- return Status::RuntimeError(
- "location mapping command returned invalid empty location");
- }
- if (!IsValidLocation(location_temp)) {
- return Status::RuntimeError(
- "location mapping command returned invalid location",
- location_temp);
- }
- *location = std::move(location_temp);
- return Status::OK();
-}
-
Status TSDescriptor::RegisterNew(const NodeInstancePB& instance,
const ServerRegistrationPB& registration,
+ LocationCache* location_cache,
shared_ptr<TSDescriptor>* desc) {
shared_ptr<TSDescriptor> ret(TSDescriptor::make_shared(instance.permanent_uuid()));
- RETURN_NOT_OK(ret->Register(instance, registration));
+ RETURN_NOT_OK(ret->Register(instance, registration, location_cache));
desc->swap(ret);
return Status::OK();
}
@@ -210,7 +156,8 @@ Status TSDescriptor::RegisterUnlocked(const NodeInstancePB& instance,
}
Status TSDescriptor::Register(const NodeInstancePB& instance,
- const ServerRegistrationPB& registration) {
+ const ServerRegistrationPB& registration,
+ LocationCache* location_cache) {
// Do basic registration work under the lock.
{
std::lock_guard<simple_spinlock> l(lock_);
@@ -219,8 +166,7 @@ Status TSDescriptor::Register(const NodeInstancePB& instance,
// Resolve the location outside the lock. This involves calling the location
// mapping script.
- const string& location_mapping_cmd = FLAGS_location_mapping_cmd;
- if (!location_mapping_cmd.empty()) {
+ if (PREDICT_TRUE(location_cache != nullptr)) {
// In some test scenarios the location is assigned per tablet server UUID.
// That's the case when multiple (or even all) tablet servers have the same
// IP address for their RPC endpoint.
@@ -228,9 +174,7 @@ Status TSDescriptor::Register(const NodeInstancePB& instance,
? permanent_uuid() : registration_->rpc_addresses(0).host();
TRACE(Substitute("tablet server $0: assigning location", permanent_uuid()));
string location;
- Status s = GetLocationFromLocationMappingCmd(location_mapping_cmd,
- cmd_arg,
- &location);
+ const auto s = location_cache->GetLocation(cmd_arg, &location);
TRACE(Substitute(
"tablet server $0: assigned location '$1'", permanent_uuid(), location));
diff --git a/src/kudu/master/ts_descriptor.h b/src/kudu/master/ts_descriptor.h
index 65724fe..76a5fa8 100644
--- a/src/kudu/master/ts_descriptor.h
+++ b/src/kudu/master/ts_descriptor.h
@@ -53,17 +53,7 @@ class TabletServerAdminServiceProxy;
namespace master {
-// Resolves 'host', which is the IP address or hostname of a tablet server or
-// client, into a location using the command 'cmd'. The result will be stored
-// in 'location', which must not be null. If there is an error running the
-// command or the output is invalid, an error Status will be returned.
-// TODO(wdberkeley): Refactor into a separate class and implement a caching
-// policy.
-// TODO(wdberkeley): Eventually we may want to get multiple locations at once
-// by giving the script multiple arguments (like Hadoop).
-Status GetLocationFromLocationMappingCmd(const std::string& cmd,
- const std::string& host,
- std::string* location);
+class LocationCache;
// Master-side view of a single tablet server.
//
@@ -73,6 +63,7 @@ class TSDescriptor : public enable_make_shared<TSDescriptor> {
public:
static Status RegisterNew(const NodeInstancePB& instance,
const ServerRegistrationPB& registration,
+ LocationCache* location_cache,
std::shared_ptr<TSDescriptor>* desc);
virtual ~TSDescriptor() = default;
@@ -89,7 +80,8 @@ class TSDescriptor : public enable_make_shared<TSDescriptor> {
// Register this tablet server.
Status Register(const NodeInstancePB& instance,
- const ServerRegistrationPB& registration);
+ const ServerRegistrationPB& registration,
+ LocationCache* location_cache);
const std::string &permanent_uuid() const { return permanent_uuid_; }
int64_t latest_seqno() const;
diff --git a/src/kudu/master/ts_manager.cc b/src/kudu/master/ts_manager.cc
index 4e4bebf..6bd5939 100644
--- a/src/kudu/master/ts_manager.cc
+++ b/src/kudu/master/ts_manager.cc
@@ -49,7 +49,9 @@ using strings::Substitute;
namespace kudu {
namespace master {
-TSManager::TSManager(const scoped_refptr<MetricEntity>& metric_entity) {
+TSManager::TSManager(LocationCache* location_cache,
+ const scoped_refptr<MetricEntity>& metric_entity)
+ : location_cache_(location_cache) {
METRIC_cluster_replica_skew.InstantiateFunctionGauge(
metric_entity,
Bind(&TSManager::ClusterSkew, Unretained(this)))
@@ -93,14 +95,15 @@ Status TSManager::RegisterTS(const NodeInstancePB& instance,
if (!ContainsKey(servers_by_id_, uuid)) {
shared_ptr<TSDescriptor> new_desc;
- RETURN_NOT_OK(TSDescriptor::RegisterNew(instance, registration, &new_desc));
+ RETURN_NOT_OK(TSDescriptor::RegisterNew(
+ instance, registration, location_cache_, &new_desc));
InsertOrDie(&servers_by_id_, uuid, new_desc);
LOG(INFO) << Substitute("Registered new tserver with Master: $0",
new_desc->ToString());
desc->swap(new_desc);
} else {
shared_ptr<TSDescriptor> found(FindOrDie(servers_by_id_, uuid));
- RETURN_NOT_OK(found->Register(instance, registration));
+ RETURN_NOT_OK(found->Register(instance, registration, location_cache_));
LOG(INFO) << Substitute("Re-registered known tserver with Master: $0",
found->ToString());
desc->swap(found);
diff --git a/src/kudu/master/ts_manager.h b/src/kudu/master/ts_manager.h
index b6dc306..bf52a9c 100644
--- a/src/kudu/master/ts_manager.h
+++ b/src/kudu/master/ts_manager.h
@@ -35,6 +35,8 @@ class ServerRegistrationPB;
namespace master {
+class LocationCache;
+
// Tracks the servers that the master has heard from, along with their
// last heartbeat, etc.
//
@@ -47,7 +49,11 @@ namespace master {
// This class is thread-safe.
class TSManager {
public:
- explicit TSManager(const scoped_refptr<MetricEntity>& metric_entity);
+ // 'location_cache' is a pointer to location mapping cache to use when
+ // registering tablet servers. The location cache should outlive the
+ // TSManager. 'metric_entity' is used to register metrics used by TSManager.
+ TSManager(LocationCache* location_cache,
+ const scoped_refptr<MetricEntity>& metric_entity);
virtual ~TSManager();
// Lookup the tablet server descriptor for the given instance identifier.
@@ -92,6 +98,8 @@ class TSManager {
std::string, std::shared_ptr<TSDescriptor>> TSDescriptorMap;
TSDescriptorMap servers_by_id_;
+ LocationCache* location_cache_;
+
DISALLOW_COPY_AND_ASSIGN(TSManager);
};
diff --git a/src/kudu/tools/ksck_remote-test.cc b/src/kudu/tools/ksck_remote-test.cc
index 1436155..6a42e0b 100644
--- a/src/kudu/tools/ksck_remote-test.cc
+++ b/src/kudu/tools/ksck_remote-test.cc
@@ -510,13 +510,19 @@ TEST_F(RemoteKsckTest, TestLeaderMasterDown) {
}
TEST_F(RemoteKsckTest, TestClusterWithLocation) {
- // There is no location assigned for the existing three tablet servers.
- // With the flag set, the newly added server will be assiged with location '/foo'.
const string location_cmd_path = JoinPathSegments(GetTestExecutableDirectory(),
"testdata/first_argument.sh");
const string location = "/foo";
FLAGS_location_mapping_cmd = strings::Substitute("$0 $1", location_cmd_path, location);
+ // After setting the --location_mapping_cmd flag it's necessary to restart
+ // the masters to pick up the new value.
+ for (auto idx = 0; idx < mini_cluster_->num_masters(); ++idx) {
+ auto* master = mini_cluster_->mini_master(idx);
+ master->Shutdown();
+ ASSERT_OK(master->Start());
+ }
+
ASSERT_OK(mini_cluster_->AddTabletServer());
ASSERT_EQ(4, mini_cluster_->num_tablet_servers());
@@ -530,29 +536,21 @@ TEST_F(RemoteKsckTest, TestClusterWithLocation) {
ASSERT_OK(ksck_->PrintResults());
string err_string = err_stream_.str();
- // The existing tablet servers should have location '<none>' displayed.
- for (int i = 0; i < 3; i++) {
- auto *ts = mini_cluster_->mini_tablet_server(i);
- ASSERT_STR_CONTAINS(err_string, strings::Substitute("$0 | $1 | HEALTHY | <none>",
- ts->uuid(),
- ts->bound_rpc_addr().ToString()));
+ // With the flag set and masters restarted, all tablet servers are assigned
+ // with location '/foo': both the existing ones and the newly added.
+ for (int idx = 0; idx < mini_cluster_->num_tablet_servers(); ++idx) {
+ auto *ts = mini_cluster_->mini_tablet_server(idx);
+ ASSERT_STR_CONTAINS(err_string, strings::Substitute("$0 | $1 | HEALTHY | $2",
+ ts->uuid(),
+ ts->bound_rpc_addr().ToString(),
+ location));
}
-
- // The newly added tablet server should have the assigned location displayed.
- auto *ts = mini_cluster_->mini_tablet_server(3);
- ASSERT_STR_CONTAINS(err_string, strings::Substitute("$0 | $1 | HEALTHY | $2",
- ts->uuid(),
- ts->bound_rpc_addr().ToString(),
- location));
-
ASSERT_STR_CONTAINS(err_string,
"Tablet Server Location Summary\n"
" Location | Count\n"
"----------+---------\n");
ASSERT_STR_CONTAINS(err_string,
- " <none> | 3\n");
- ASSERT_STR_CONTAINS(err_string,
- " /foo | 1\n");
+ " /foo | 4\n");
}
} // namespace tools