You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2019/10/16 20:47:02 UTC
[kudu] branch master updated (d525c11 -> adecebd)
This is an automated email from the ASF dual-hosted git repository.
adar pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.
from d525c11 master: fix the value of table metric 'live_row_count'
new 7e8f37f kudu-tool-test: deflake TestFsAddRemoveDataDirEndToEnd
new adecebd master: GetTableStatistics should use signed ints
The 2 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/table_statistics-internal.h | 6 +++---
src/kudu/fs/data_dirs.cc | 17 +++++++++++++----
src/kudu/master/master.proto | 4 ++--
src/kudu/tools/kudu-tool-test.cc | 27 +++++++++++++++++++++++++--
4 files changed, 43 insertions(+), 11 deletions(-)
[kudu] 01/02: kudu-tool-test: deflake TestFsAddRemoveDataDirEndToEnd
Posted by ad...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 7e8f37f8ddf46b965b0af4eba3f31c02b11b8fba
Author: Adar Dembo <ad...@cloudera.com>
AuthorDate: Wed Oct 16 12:15:56 2019 -0700
kudu-tool-test: deflake TestFsAddRemoveDataDirEndToEnd
This test was made somewhat flaky by KUDU-2901. The reason is subtle: if any
unrelated IO (such as from a concurrently running test) interleaves with the
creation of the second table, it's possible for the new data directory to be
measured with less available space than the existing ones. The existing LBM
containers have all been preallocated by this point so the test never
remeasures available space. Thus, when the second table flushes, the
KUDU-2901 heuristic ensures that all new data blocks land on the existing
data directories rather than the new one.
The fix is simple: a feature flag for this feature (which we should have had
anyway), which we disable for the test.
Change-Id: Ifb92759318c12d791d26eb8fb6b77914c284f4cb
Reviewed-on: http://gerrit.cloudera.org:8080/14462
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
src/kudu/fs/data_dirs.cc | 17 +++++++++++++----
src/kudu/tools/kudu-tool-test.cc | 6 ++++++
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/kudu/fs/data_dirs.cc b/src/kudu/fs/data_dirs.cc
index c5205ab..5812e1d 100644
--- a/src/kudu/fs/data_dirs.cc
+++ b/src/kudu/fs/data_dirs.cc
@@ -95,6 +95,12 @@ DEFINE_bool(fs_lock_data_dirs, true,
TAG_FLAG(fs_lock_data_dirs, unsafe);
TAG_FLAG(fs_lock_data_dirs, evolving);
+DEFINE_bool(fs_data_dirs_consider_available_space, true,
+ "Whether to consider available space when selecting a data "
+ "directory during tablet or data block creation.");
+TAG_FLAG(fs_data_dirs_consider_available_space, runtime);
+TAG_FLAG(fs_data_dirs_consider_available_space, evolving);
+
METRIC_DEFINE_gauge_uint64(server, data_dirs_failed,
"Data Directories Failed",
kudu::MetricUnit::kDataDirectories,
@@ -1011,9 +1017,11 @@ Status DataDirManager::GetDirForBlock(const CreateBlockOptions& opts, DataDir**
return Status::OK();
}
// Pick two randomly and select the one with more space.
- shuffle(candidate_dirs.begin(), candidate_dirs.end(), default_random_engine(rng_.Next()));
- *dir = (candidate_dirs[0]->available_bytes() > candidate_dirs[1]->available_bytes()) ?
- candidate_dirs[0] : candidate_dirs[1];
+ shuffle(candidate_dirs.begin(), candidate_dirs.end(),
+ default_random_engine(rng_.Next()));
+ *dir = PREDICT_TRUE(FLAGS_fs_data_dirs_consider_available_space) &&
+ candidate_dirs[0]->available_bytes() > candidate_dirs[1]->available_bytes() ?
+ candidate_dirs[0] : candidate_dirs[1];
return Status::OK();
}
@@ -1130,7 +1138,8 @@ void DataDirManager::GetDirsForGroupUnlocked(int target_size,
int tablets_in_first = FindOrDie(tablets_by_uuid_idx_map_, candidate_indices[0]).size();
int tablets_in_second = FindOrDie(tablets_by_uuid_idx_map_, candidate_indices[1]).size();
int selected_index = 0;
- if (tablets_in_first == tablets_in_second) {
+ if (tablets_in_first == tablets_in_second &&
+ PREDICT_TRUE(FLAGS_fs_data_dirs_consider_available_space)) {
int64_t space_in_first = FindOrDie(data_dir_by_uuid_idx_,
candidate_indices[0])->available_bytes();
int64_t space_in_second = FindOrDie(data_dir_by_uuid_idx_,
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index d7db521..717d284 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -136,6 +136,7 @@
#include "kudu/util/test_util.h"
#include "kudu/util/url-coding.h"
+DECLARE_bool(fs_data_dirs_consider_available_space);
DECLARE_bool(hive_metastore_sasl_enabled);
DECLARE_bool(show_values);
DECLARE_bool(show_attributes);
@@ -4943,6 +4944,11 @@ TEST_F(ToolTest, TestFsAddRemoveDataDirEndToEnd) {
return;
}
+ // Disable the available space heuristic as it can interfere with the desired
+ // test invariant below (that the newly created table write to the newly added
+ // data directory).
+ FLAGS_fs_data_dirs_consider_available_space = false;
+
// Start a cluster whose tserver has multiple data directories.
InternalMiniClusterOptions opts;
opts.num_data_dirs = 2;
[kudu] 02/02: master: GetTableStatistics should use signed ints
Posted by ad...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
adar pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit adecebd9e22b50254d69c94c784d5cba3657890b
Author: Adar Dembo <ad...@cloudera.com>
AuthorDate: Wed Oct 16 13:12:27 2019 -0700
master: GetTableStatistics should use signed ints
The statistics are reported to the master as signed ints, so the master
should likewise advertise them as such. Otherwise the live_row_count value
of -1 (when a tablet doesn't support it) shows up as:
TABLE test-workload
on disk size: 0
live row count: 18446744073709551615
The public APIs were already treating them as signed; they were only only
unsigned on the wire.
Change-Id: I398cb85888bdc9463264692911788de2fef67dfc
Reviewed-on: http://gerrit.cloudera.org:8080/14463
Reviewed-by: Grant Henke <gr...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
src/kudu/client/table_statistics-internal.h | 6 +++---
src/kudu/master/master.proto | 4 ++--
src/kudu/tools/kudu-tool-test.cc | 21 +++++++++++++++++++--
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/src/kudu/client/table_statistics-internal.h b/src/kudu/client/table_statistics-internal.h
index 153e9f9..eda125d 100644
--- a/src/kudu/client/table_statistics-internal.h
+++ b/src/kudu/client/table_statistics-internal.h
@@ -31,7 +31,7 @@ using strings::Substitute;
class KuduTableStatistics::Data {
public:
- Data(uint64_t on_disk_size, uint64_t live_row_count)
+ Data(int64_t on_disk_size, int64_t live_row_count)
: on_disk_size_(on_disk_size),
live_row_count_(live_row_count) {
}
@@ -46,8 +46,8 @@ class KuduTableStatistics::Data {
return display_string;
}
- const uint64_t on_disk_size_;
- const uint64_t live_row_count_;
+ const int64_t on_disk_size_;
+ const int64_t live_row_count_;
private:
DISALLOW_COPY_AND_ASSIGN(Data);
diff --git a/src/kudu/master/master.proto b/src/kudu/master/master.proto
index 0c77150..bda76e2 100644
--- a/src/kudu/master/master.proto
+++ b/src/kudu/master/master.proto
@@ -553,8 +553,8 @@ message GetTableStatisticsResponsePB {
optional MasterErrorPB error = 1;
// The table statistics from table metrics.
- optional uint64 on_disk_size = 2;
- optional uint64 live_row_count = 3;
+ optional int64 on_disk_size = 2;
+ optional int64 live_row_count = 3;
}
message GetTableLocationsRequestPB {
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index 717d284..f122e78 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -5487,7 +5487,6 @@ TEST_F(AuthzTServerChecksumTest, TestAuthorizeChecksum) {
"--checksum_scan"
};
ASSERT_OK(RunKuduTool(checksum_args));
-
}
// Regression test for KUDU-2851.
@@ -5512,7 +5511,6 @@ TEST_F(ToolTest, TestFailedTableScan) {
ASSERT_TRUE(s.IsRuntimeError());
SCOPED_TRACE(stderr);
ASSERT_STR_CONTAINS(stderr, "Timed out");
-
}
TEST_F(ToolTest, TestFailedTableCopy) {
@@ -5541,7 +5539,26 @@ TEST_F(ToolTest, TestFailedTableCopy) {
ASSERT_TRUE(s.IsRuntimeError());
SCOPED_TRACE(stderr);
ASSERT_STR_CONTAINS(stderr, "Timed out");
+}
+
+TEST_F(ToolTest, TestGetTableStatisticsLiveRowCountNotSupported) {
+ ExternalMiniClusterOptions opts;
+ opts.extra_master_flags = { "--mock_table_metrics_for_testing=true",
+ "--live_row_count_for_testing=-1" };
+ NO_FATALS(StartExternalMiniCluster(std::move(opts)));
+
+ // Create an empty table.
+ TestWorkload workload(cluster_.get());
+ workload.set_num_replicas(1);
+ workload.Setup();
+ string stdout;
+ NO_FATALS(RunActionStdoutString(
+ Substitute("table statistics $0 $1",
+ cluster_->master()->bound_rpc_addr().ToString(),
+ TestWorkload::kDefaultTableName),
+ &stdout));
+ ASSERT_STR_CONTAINS(stdout, "live row count: -1");
}
} // namespace tools