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 2017/06/16 21:43:59 UTC
[1/3] kudu git commit: [registration-test] fix flake in
TestTabletReports
Repository: kudu
Updated Branches:
refs/heads/master 834bb35e8 -> c0798a942
[registration-test] fix flake in TestTabletReports
Prior to the fix, the failure rate attributed to this flake was about
0.3% in 1K run:
http://dist-test.cloudera.org//job?job_id=aserbin.1497582474.17739
After the fix, no failures in 1K run:
http://dist-test.cloudera.org//job?job_id=aserbin.1497629325.23427
Both runs were of DEBUG build run with -stress_cpu_threads=8 flag.
The crux of the fix is to use AssertEvetually instead of relying on a
hard-coded delay to capture system catalog's metric. That's because
the metrics of interest are updated upon processing tablet reports from
tservers which contain tablet consensus status information. The latter
are sent with a tserver-->master heartbeat when the tserver determines
that it has become the leader for one of its tablets.
In addition to that, this patch includes a clean-up of the test code.
Change-Id: I906465ad220236538175c80972ae055193f9bb45
Reviewed-on: http://gerrit.cloudera.org:8080/7209
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/722f8039
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/722f8039
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/722f8039
Branch: refs/heads/master
Commit: 722f80399a748478c295d180459d1136c59edc14
Parents: 834bb35
Author: Alexey Serbin <as...@cloudera.com>
Authored: Fri Jun 16 09:18:07 2017 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Fri Jun 16 21:23:23 2017 +0000
----------------------------------------------------------------------
src/kudu/integration-tests/registration-test.cc | 108 ++++++++++---------
1 file changed, 58 insertions(+), 50 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/722f8039/src/kudu/integration-tests/registration-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/registration-test.cc b/src/kudu/integration-tests/registration-test.cc
index a478616..4ae7bd3 100644
--- a/src/kudu/integration-tests/registration-test.cc
+++ b/src/kudu/integration-tests/registration-test.cc
@@ -15,10 +15,13 @@
// specific language governing permissions and limitations
// under the License.
-#include <gflags/gflags.h>
-#include <gtest/gtest.h>
#include <memory>
#include <string>
+#include <utility>
+#include <vector>
+
+#include <gflags/gflags_declare.h>
+#include <gtest/gtest.h>
#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol-test-util.h"
@@ -51,11 +54,11 @@ DECLARE_int32(heartbeat_interval_ms);
METRIC_DECLARE_counter(rows_inserted);
METRIC_DECLARE_counter(rows_updated);
-namespace kudu {
-
-using std::vector;
using std::shared_ptr;
using std::string;
+using std::vector;
+
+namespace kudu {
using master::CatalogManager;
using master::CreateTableRequestPB;
@@ -73,7 +76,7 @@ using tserver::MiniTabletServer;
void CreateTableForTesting(MiniMaster* mini_master,
const string& table_name,
const Schema& schema,
- string *tablet_id) {
+ string* tablet_id) {
{
CreateTableRequestPB req;
CreateTableResponsePB resp;
@@ -84,7 +87,7 @@ void CreateTableForTesting(MiniMaster* mini_master,
CatalogManager* catalog = mini_master->master()->catalog_manager();
CatalogManager::ScopedLeaderSharedLock l(catalog);
ASSERT_OK(l.first_failed_status());
- ASSERT_OK(catalog->CreateTable(&req, &resp, NULL));
+ ASSERT_OK(catalog->CreateTable(&req, &resp, nullptr));
}
int wait_time = 1000;
@@ -135,20 +138,20 @@ void CreateTableForTesting(MiniMaster* mini_master,
class RegistrationTest : public KuduTest {
public:
RegistrationTest()
- : schema_({ ColumnSchema("c1", UINT32) }, 1) {
+ : schema_({ ColumnSchema("c1", UINT32) }, 1) {
}
- virtual void SetUp() OVERRIDE {
+ void SetUp() override {
// Make heartbeats faster to speed test runtime.
FLAGS_heartbeat_interval_ms = 10;
KuduTest::SetUp();
- cluster_.reset(new MiniCluster(env_, MiniClusterOptions()));
+ cluster_.reset(new MiniCluster(env_, {}));
ASSERT_OK(cluster_->Start());
}
- virtual void TearDown() OVERRIDE {
+ void TearDown() override {
cluster_->Shutdown();
}
@@ -175,26 +178,32 @@ class RegistrationTest : public KuduTest {
Status WaitForReplicaCount(const string& tablet_id,
int expected_count,
- TabletLocationsPB* locations) {
+ TabletLocationsPB* locations = nullptr) {
while (true) {
master::CatalogManager* catalog =
cluster_->mini_master()->master()->catalog_manager();
Status s;
+ TabletLocationsPB loc;
do {
master::CatalogManager::ScopedLeaderSharedLock l(catalog);
const Status& ls = l.first_failed_status();
- if (ls.IsServiceUnavailable()) {
+ if (ls.IsServiceUnavailable() || ls.IsIllegalState()) {
// ServiceUnavailable means catalog manager is not yet ready
- // to serve requests -- try again later.
+ // to serve requests; IllegalState means the catalog is not the
+ // leader yet. That's an indication of a transient state where it's
+ // it's necessary to try again later.
break; // exiting out of the 'do {...} while (false)' scope
}
RETURN_NOT_OK(ls);
- s = catalog->GetTabletLocations(tablet_id, locations);
+ s = catalog->GetTabletLocations(tablet_id, &loc);
} while (false);
- if (s.ok() && locations->replicas_size() == expected_count) {
+ if (s.ok() && loc.replicas_size() == expected_count) {
+ if (locations) {
+ *locations = std::move(loc);
+ }
return Status::OK();
}
- SleepFor(MonoDelta::FromMilliseconds(1));
+ SleepFor(MonoDelta::FromMilliseconds(10));
}
}
@@ -215,14 +224,14 @@ TEST_F(RegistrationTest, TestTSRegisters) {
descs[0]->GetRegistration(®);
{
SCOPED_TRACE(SecureShortDebugString(reg));
- ASSERT_EQ(SecureShortDebugString(reg).find("0.0.0.0"), string::npos)
- << "Should not include wildcards in registration";
+ ASSERT_EQ(string::npos, SecureShortDebugString(reg).find("0.0.0.0"))
+ << "Should not include wildcards in registration";
}
ASSERT_NO_FATAL_FAILURE(CheckTabletServersPage());
// Restart the master, so it loses the descriptor, and ensure that the
- // hearbeater thread handles re-registering.
+ // heartbeater thread handles re-registering.
cluster_->mini_master()->Shutdown();
ASSERT_OK(cluster_->mini_master()->Restart());
@@ -255,62 +264,61 @@ TEST_F(RegistrationTest, TestMultipleTS) {
// to something more appropriate - doesn't seem worth having separate
// whole test suites for registration, tablet reports, etc.
TEST_F(RegistrationTest, TestTabletReports) {
- string tablet_id_1;
- string tablet_id_2;
-
- MiniTabletServer* ts = cluster_->mini_tablet_server(0);
- string ts_root = cluster_->GetTabletServerFsRoot(0);
-
auto GetCatalogMetric = [&](CounterPrototype& prototype) {
- auto metrics = cluster_->mini_master()->master()->catalog_manager()->sys_catalog()
- ->tablet_replica()->tablet()->GetMetricEntity();
+ auto metrics = cluster_->mini_master()->master()->catalog_manager()->
+ sys_catalog()->tablet_replica()->tablet()->GetMetricEntity();
return prototype.Instantiate(metrics)->value();
};
- int startup_rows_inserted = GetCatalogMetric(METRIC_rows_inserted);
+ const int startup_rows_inserted = GetCatalogMetric(METRIC_rows_inserted);
// Add a table, make sure it reports itself.
- CreateTableForTesting(cluster_->mini_master(), "fake-table", schema_, &tablet_id_1);
-
- TabletLocationsPB locs;
- ASSERT_OK(WaitForReplicaCount(tablet_id_1, 1, &locs));
- ASSERT_EQ(1, locs.replicas_size());
- LOG(INFO) << "Tablet successfully reported on " << locs.replicas(0).ts_info().permanent_uuid();
+ string tablet_id_1;
+ NO_FATALS(CreateTableForTesting(
+ cluster_->mini_master(), "tablet-reports-1", schema_, &tablet_id_1));
+ TabletLocationsPB locs_1;
+ ASSERT_OK(WaitForReplicaCount(tablet_id_1, 1, &locs_1));
+ ASSERT_EQ(1, locs_1.replicas_size());
// Check that we inserted the right number of rows for the new single-tablet table
// (one for the table, one for the tablet).
- int post_create_rows_inserted = GetCatalogMetric(METRIC_rows_inserted);
+ const int post_create_rows_inserted = GetCatalogMetric(METRIC_rows_inserted);
EXPECT_EQ(2, post_create_rows_inserted - startup_rows_inserted)
<< "Should have inserted one row each for the table and tablet";
// Add another table, make sure it is reported via incremental.
- CreateTableForTesting(cluster_->mini_master(), "fake-table2", schema_, &tablet_id_2);
- ASSERT_OK(WaitForReplicaCount(tablet_id_2, 1, &locs));
+ string tablet_id_2;
+ NO_FATALS(CreateTableForTesting(
+ cluster_->mini_master(), "tablet-reports-2", schema_, &tablet_id_2));
+ ASSERT_OK(WaitForReplicaCount(tablet_id_2, 1));
// Shut down the whole system, bring it back up, and make sure the tablets
// are reported.
- ts->Shutdown();
- cluster_->mini_master()->Shutdown();
- ASSERT_OK(cluster_->mini_master()->Restart());
- ASSERT_OK(ts->Start());
-
- ASSERT_OK(WaitForReplicaCount(tablet_id_1, 1, &locs));
- ASSERT_OK(WaitForReplicaCount(tablet_id_2, 1, &locs));
-
- SleepFor(MonoDelta::FromSeconds(1));
+ cluster_->Shutdown();
+ ASSERT_OK(cluster_->Start());
// After restart, check that the tablet reports produced the expected number of
// writes to the catalog table:
// - No inserts, because there are no new tablets.
- // - Two updates, since both replicas should have increased their term on restart.
- EXPECT_EQ(0, GetCatalogMetric(METRIC_rows_inserted));
- EXPECT_EQ(2, GetCatalogMetric(METRIC_rows_updated));
+ // - Two updates, since both replicas increase their term on restart.
+ //
+ // It can take some time for the TS to re-heartbeat. To avoid flakiness, here
+ // it's easier to wait for the target non-zero metric value instead of
+ // hard-coding an extra delay.
+ AssertEventually([&]() {
+ ASSERT_EQ(0, GetCatalogMetric(METRIC_rows_inserted));
+ ASSERT_EQ(2, GetCatalogMetric(METRIC_rows_updated));
+ });
// If we restart just the master, it should not write any data to the catalog, since the
// tablets themselves are not changing term, etc.
cluster_->mini_master()->Shutdown();
ASSERT_OK(cluster_->mini_master()->Restart());
+
// Sleep for a second to make sure the TS has plenty of time to re-heartbeat.
+ // The metrics are updated after processing TS heartbeats, and we want to
+ // capture updated metric readings.
SleepFor(MonoDelta::FromSeconds(1));
+
EXPECT_EQ(0, GetCatalogMetric(METRIC_rows_inserted));
EXPECT_EQ(0, GetCatalogMetric(METRIC_rows_updated));
}
[3/3] kudu git commit: KUDU-2003. Fix fd-cache related tests on
high-core systems
Posted by al...@apache.org.
KUDU-2003. Fix fd-cache related tests on high-core systems
These two tests would fail on systems with a number of cores that was
not a divisor of 192. This patch switches the tests to use a non-sharded
LRU cache so that the behavior is not dependent on the core count.
I verified that these tests now pass on a machine with 88 logical cores.
Change-Id: Ibd901730f16c70bb2e4a60464ab08a9a6f14c32f
Reviewed-on: http://gerrit.cloudera.org:8080/7160
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c0798a94
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c0798a94
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c0798a94
Branch: refs/heads/master
Commit: c0798a9426581629d285ebf164f112d9a6a2f7ea
Parents: 5808e6d
Author: Todd Lipcon <to...@cloudera.com>
Authored: Mon Jun 12 15:57:45 2017 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Fri Jun 16 21:27:53 2017 +0000
----------------------------------------------------------------------
src/kudu/fs/block_manager-stress-test.cc | 5 ++++
src/kudu/util/file_cache-stress-test.cc | 35 +++++++++++++++------------
2 files changed, 24 insertions(+), 16 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0798a94/src/kudu/fs/block_manager-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager-stress-test.cc b/src/kudu/fs/block_manager-stress-test.cc
index 55de36c..ac23296 100644
--- a/src/kudu/fs/block_manager-stress-test.cc
+++ b/src/kudu/fs/block_manager-stress-test.cc
@@ -37,6 +37,7 @@
#include "kudu/util/test_util.h"
#include "kudu/util/thread.h"
+DECLARE_bool(cache_force_single_shard);
DECLARE_double(log_container_excess_space_before_cleanup_fraction);
DECLARE_double(log_container_live_metadata_before_compact_ratio);
DECLARE_int64(block_manager_max_open_files);
@@ -115,6 +116,10 @@ class BlockManagerStressTest : public KuduTest {
// Compact block manager metadata aggressively.
FLAGS_log_container_live_metadata_before_compact_ratio = 0.99;
+ // Use a single cache shard. Otherwise, the cache can be a little bit "sloppy"
+ // depending on the number of CPUs on the system.
+ FLAGS_cache_force_single_shard = true;
+
if (FLAGS_block_manager_paths.empty()) {
data_dirs_.push_back(test_dir_);
} else {
http://git-wip-us.apache.org/repos/asf/kudu/blob/c0798a94/src/kudu/util/file_cache-stress-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/file_cache-stress-test.cc b/src/kudu/util/file_cache-stress-test.cc
index 966f202..807d85e 100644
--- a/src/kudu/util/file_cache-stress-test.cc
+++ b/src/kudu/util/file_cache-stress-test.cc
@@ -54,7 +54,7 @@
const Status& _s = (to_call); \
if (!_s.ok()) { \
LOG(INFO) << "Dumping cache contents"; \
- vector<string> lines = strings::Split(cache_.ToDebugString(), "\n", \
+ vector<string> lines = strings::Split(cache_->ToDebugString(), "\n", \
strings::SkipEmpty()); \
for (const auto& l : lines) { \
LOG(INFO) << l; \
@@ -63,15 +63,12 @@
CHECK(_s.ok()) << "Bad status: " << _s.ToString(); \
} while (0);
-// This default value is friendly to many n-CPU configurations.
-DEFINE_int32(test_max_open_files, 192, "Maximum number of open files enforced "
- "by the cache. Should be a multiple of the number of CPUs on the "
- "system.");
-
DEFINE_int32(test_num_producer_threads, 1, "Number of producer threads");
DEFINE_int32(test_num_consumer_threads, 4, "Number of consumer threads");
DEFINE_int32(test_duration_secs, 2, "Number of seconds to run the test");
+DECLARE_bool(cache_force_single_shard);
+
using std::deque;
using std::shared_ptr;
using std::thread;
@@ -82,22 +79,28 @@ using strings::Substitute;
namespace kudu {
+// FD limit to enforce during the test.
+static const int kTestMaxOpenFiles = 100;
+
template <class FileType>
class FileCacheStressTest : public KuduTest {
public:
typedef unordered_map<string, unordered_map<string, int>> MetricMap;
FileCacheStressTest()
- : cache_("test",
- env_,
- FLAGS_test_max_open_files,
- scoped_refptr<MetricEntity>()),
- rand_(SeedRandom()),
+ : rand_(SeedRandom()),
running_(1) {
+ // Use a single shard. Otherwise, the cache can be a little bit "sloppy"
+ // depending on the number of CPUs on the system.
+ FLAGS_cache_force_single_shard = true;
+ cache_.reset(new FileCache<FileType>("test",
+ env_,
+ kTestMaxOpenFiles,
+ scoped_refptr<MetricEntity>()));
}
void SetUp() override {
- ASSERT_OK(cache_.Init());
+ ASSERT_OK(cache_->Init());
}
void ProducerThread() {
@@ -153,7 +156,7 @@ class FileCacheStressTest : public KuduTest {
continue;
}
shared_ptr<FileType> new_file;
- TEST_CHECK_OK(cache_.OpenExistingFile(to_open, &new_file));
+ TEST_CHECK_OK(cache_->OpenExistingFile(to_open, &new_file));
FinishedOpen(to_open);
metrics[BaseName(to_open)]["open"]++;
files.emplace_back(new_file);
@@ -177,7 +180,7 @@ class FileCacheStressTest : public KuduTest {
if (!GetRandomFile(DELETE, &rand, &to_delete)) {
continue;
}
- TEST_CHECK_OK(cache_.DeleteFile(to_delete));
+ TEST_CHECK_OK(cache_->DeleteFile(to_delete));
metrics[BaseName(to_delete)]["delete"]++;
}
} while (!running_.WaitFor(MonoDelta::FromMilliseconds(1)));
@@ -281,7 +284,7 @@ class FileCacheStressTest : public KuduTest {
}
}
- FileCache<FileType> cache_;
+ unique_ptr<FileCache<FileType>> cache_;
// Used to seed per-thread PRNGs.
ThreadSafeRandom rand_;
@@ -343,7 +346,7 @@ TYPED_TEST(FileCacheStressTest, TestStress) {
// Start the threads.
PeriodicOpenFdChecker checker(
this->env_,
- FLAGS_test_max_open_files + // cache capacity
+ kTestMaxOpenFiles + // cache capacity
FLAGS_test_num_producer_threads + // files being written
FLAGS_test_num_consumer_threads); // files being opened
checker.Start();
[2/3] kudu git commit: Make AssertEventually compatible with
--gtest_break_on_failure
Posted by al...@apache.org.
Make AssertEventually compatible with --gtest_break_on_failure
ASSERT_EVENTUALLY(...) works by suppressing assertion failures for a
certain number of retries until the specified assertions eventually
succeed. With gtest_break_on_failure, the failed assertions would cause
the test to crash even though they were expected.
Tested with subprocess-test --gtest_break_on_failure
Change-Id: I8e5299c2bb200420cf5ecf6aea8e13d2e44a1e43
Reviewed-on: http://gerrit.cloudera.org:8080/7212
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/5808e6d9
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/5808e6d9
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/5808e6d9
Branch: refs/heads/master
Commit: 5808e6d9fe2ef278cef2b6bd6b87a43d8f69d82d
Parents: 722f803
Author: Todd Lipcon <to...@apache.org>
Authored: Fri Jun 16 13:53:56 2017 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Fri Jun 16 21:27:19 2017 +0000
----------------------------------------------------------------------
src/kudu/util/test_util.cc | 53 +++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/5808e6d9/src/kudu/util/test_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 50c80c6..af809a7 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -32,6 +32,7 @@
#include "kudu/util/env.h"
#include "kudu/util/path_util.h"
#include "kudu/util/random.h"
+#include "kudu/util/scoped_cleanup.h"
#include "kudu/util/spinlock_profiling.h"
DEFINE_string(test_leave_files, "on_failure",
@@ -224,28 +225,38 @@ string GetTestDataDirectory() {
void AssertEventually(const std::function<void(void)>& f,
const MonoDelta& timeout) {
const MonoTime deadline = MonoTime::Now() + timeout;
-
- for (int attempts = 0; MonoTime::Now() < deadline; attempts++) {
- // Capture any assertion failures within this scope (i.e. from their function)
- // into 'results'
- testing::TestPartResultArray results;
- testing::ScopedFakeTestPartResultReporter reporter(
- testing::ScopedFakeTestPartResultReporter::INTERCEPT_ONLY_CURRENT_THREAD,
- &results);
- f();
-
- // Determine whether their function produced any new test failure results.
- bool has_failures = false;
- for (int i = 0; i < results.size(); i++) {
- has_failures |= results.GetTestPartResult(i).failed();
- }
- if (!has_failures) {
- return;
+ {
+ // Disable --gtest_break_on_failure, or else the assertion failures
+ // inside our attempts will cause the test to SEGV even though we
+ // would like to retry.
+ bool old_break_on_failure = testing::FLAGS_gtest_break_on_failure;
+ auto c = MakeScopedCleanup([old_break_on_failure]() {
+ testing::FLAGS_gtest_break_on_failure = old_break_on_failure;
+ });
+ testing::FLAGS_gtest_break_on_failure = false;
+
+ for (int attempts = 0; MonoTime::Now() < deadline; attempts++) {
+ // Capture any assertion failures within this scope (i.e. from their function)
+ // into 'results'
+ testing::TestPartResultArray results;
+ testing::ScopedFakeTestPartResultReporter reporter(
+ testing::ScopedFakeTestPartResultReporter::INTERCEPT_ONLY_CURRENT_THREAD,
+ &results);
+ f();
+
+ // Determine whether their function produced any new test failure results.
+ bool has_failures = false;
+ for (int i = 0; i < results.size(); i++) {
+ has_failures |= results.GetTestPartResult(i).failed();
+ }
+ if (!has_failures) {
+ return;
+ }
+
+ // If they had failures, sleep and try again.
+ int sleep_ms = (attempts < 10) ? (1 << attempts) : 1000;
+ SleepFor(MonoDelta::FromMilliseconds(sleep_ms));
}
-
- // If they had failures, sleep and try again.
- int sleep_ms = (attempts < 10) ? (1 << attempts) : 1000;
- SleepFor(MonoDelta::FromMilliseconds(sleep_ms));
}
// If we ran out of time looping, run their function one more time