You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2017/04/10 17:28:44 UTC
[2/2] kudu git commit: KUDU-1966: Data directories can be removed
erroneously
KUDU-1966: Data directories can be removed erroneously
Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.
Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Reviewed-on: http://gerrit.cloudera.org:8080/6589
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/bd24f04f
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/bd24f04f
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/bd24f04f
Branch: refs/heads/master
Commit: bd24f04fb43db9f1fcbf9a60ecc31824c3c79bfd
Parents: 5f1ca4f
Author: Dan Burkert <da...@apache.org>
Authored: Fri Apr 7 08:56:24 2017 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Mon Apr 10 17:24:43 2017 +0000
----------------------------------------------------------------------
src/kudu/fs/block_manager_util-test.cc | 24 +++++++---
src/kudu/fs/block_manager_util.cc | 70 ++++++++++++++++++-----------
2 files changed, 60 insertions(+), 34 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/bd24f04f/src/kudu/fs/block_manager_util-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager_util-test.cc b/src/kudu/fs/block_manager_util-test.cc
index ee19638..f44cbca 100644
--- a/src/kudu/fs/block_manager_util-test.cc
+++ b/src/kudu/fs/block_manager_util-test.cc
@@ -108,7 +108,7 @@ static void RunCheckIntegrityTest(Env* env,
int i = 0;
for (const PathSetPB& ps : path_sets) {
gscoped_ptr<PathInstanceMetadataFile> instance(
- new PathInstanceMetadataFile(env, "asdf", Substitute("$0", i)));
+ new PathInstanceMetadataFile(env, "asdf", Substitute("/tmp/$0/instance", i)));
gscoped_ptr<PathInstanceMetadataPB> metadata(new PathInstanceMetadataPB());
metadata->set_block_manager_type("asdf");
metadata->set_filesystem_block_size_bytes(1);
@@ -144,7 +144,8 @@ TEST_F(KuduTest, CheckIntegrity) {
path_sets_copy[1].set_uuid(path_sets_copy[0].uuid());
EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest(
env_, path_sets_copy,
- "IO error: File 1 claimed uuid fee already claimed by file 0"));
+ "IO error: Data directories /tmp/0 and /tmp/1 have duplicate instance metadata UUIDs: "
+ "fee"));
}
{
// Test where the path sets have duplicate UUIDs.
@@ -154,7 +155,8 @@ TEST_F(KuduTest, CheckIntegrity) {
}
EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest(
env_, path_sets_copy,
- "IO error: File 0 has duplicate uuids: fee,fi,fo,fum,fee"));
+ "IO error: Data directory /tmp/0 instance metadata path set contains duplicate UUIDs: "
+ "fee,fi,fo,fum,fee"));
}
{
// Test where a path set claims a UUID that isn't in all_uuids.
@@ -162,8 +164,8 @@ TEST_F(KuduTest, CheckIntegrity) {
path_sets_copy[1].set_uuid("something_else");
EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest(
env_, path_sets_copy,
- "IO error: File 1 claimed uuid something_else which is not in "
- "all_uuids (fee,fi,fo,fum)"));
+ "IO error: Data directory /tmp/1 instance metadata contains unexpected UUID: "
+ "something_else"));
}
{
// Test where a path set claims a different all_uuids.
@@ -171,8 +173,16 @@ TEST_F(KuduTest, CheckIntegrity) {
path_sets_copy[1].add_all_uuids("another_uuid");
EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest(
env_, path_sets_copy,
- "IO error: File 1 claimed all_uuids fee,fi,fo,fum,another_uuid but "
- "file 0 claimed all_uuids fee,fi,fo,fum"));
+ "IO error: Data directories /tmp/0 and /tmp/1 have different instance metadata UUID sets: "
+ "fee,fi,fo,fum vs fee,fi,fo,fum,another_uuid"));
+ }
+ {
+ // Test removing a path from the set.
+ vector<PathSetPB> path_sets_copy(path_sets);
+ path_sets_copy.resize(1);
+ EXPECT_NO_FATAL_FAILURE(RunCheckIntegrityTest(
+ env_, path_sets_copy,
+ "IO error: 1 data directories provided, but expected 4"));
}
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/bd24f04f/src/kudu/fs/block_manager_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager_util.cc b/src/kudu/fs/block_manager_util.cc
index 79c13b5..8be6cde 100644
--- a/src/kudu/fs/block_manager_util.cc
+++ b/src/kudu/fs/block_manager_util.cc
@@ -124,47 +124,63 @@ Status PathInstanceMetadataFile::Unlock() {
Status PathInstanceMetadataFile::CheckIntegrity(
const vector<PathInstanceMetadataFile*>& instances) {
+ CHECK(!instances.empty());
+
+ // Note: although this verification works at the level of UUIDs and instance
+ // files, the (user-facing) error messages are reported in terms of data
+ // directories, because UUIDs and instance files are internal details.
+
+ // Map of instance UUID to path instance structure. Tracks duplicate UUIDs.
unordered_map<string, PathInstanceMetadataFile*> uuids;
- pair<string, PathInstanceMetadataFile*> first_all_uuids;
+
+ // Set of UUIDs specified in the path set of the first instance. All instances
+ // will be compared against this one to make sure all path sets match.
+ set<string> all_uuids(instances[0]->metadata()->path_set().all_uuids().begin(),
+ instances[0]->metadata()->path_set().all_uuids().end());
+
+ if (all_uuids.size() != instances.size()) {
+ return Status::IOError(
+ Substitute("$0 data directories provided, but expected $1",
+ instances.size(), all_uuids.size()));
+ }
for (PathInstanceMetadataFile* instance : instances) {
const PathSetPB& path_set = instance->metadata()->path_set();
- // Check that this instance's UUID wasn't already claimed.
- PathInstanceMetadataFile** other =
- InsertOrReturnExisting(&uuids, path_set.uuid(), instance);
+ // Check that the instance's UUID has not been claimed by another instance.
+ PathInstanceMetadataFile** other = InsertOrReturnExisting(&uuids, path_set.uuid(), instance);
if (other) {
- return Status::IOError(Substitute(
- "File $0 claimed uuid $1 already claimed by file $2",
- instance->filename_, path_set.uuid(), (*other)->filename_));
+ return Status::IOError(
+ Substitute("Data directories $0 and $1 have duplicate instance metadata UUIDs",
+ (*other)->path(), instance->path()),
+ path_set.uuid());
}
- // Check that there are no duplicate UUIDs in all_uuids.
+ // Check that the instance's UUID is a member of all_uuids.
+ if (!ContainsKey(all_uuids, path_set.uuid())) {
+ return Status::IOError(
+ Substitute("Data directory $0 instance metadata contains unexpected UUID",
+ instance->path()),
+ path_set.uuid());
+ }
+
+ // Check that the instance's UUID set does not contain duplicates.
set<string> deduplicated_uuids(path_set.all_uuids().begin(),
path_set.all_uuids().end());
string all_uuids_str = JoinStrings(path_set.all_uuids(), ",");
if (deduplicated_uuids.size() != path_set.all_uuids_size()) {
- return Status::IOError(Substitute(
- "File $0 has duplicate uuids: $1",
- instance->filename_, all_uuids_str));
- }
-
- // Check that this instance's UUID is a member of all_uuids.
- if (!ContainsKey(deduplicated_uuids, path_set.uuid())) {
- return Status::IOError(Substitute(
- "File $0 claimed uuid $1 which is not in all_uuids ($2)",
- instance->filename_, path_set.uuid(), all_uuids_str));
+ return Status::IOError(
+ Substitute("Data directory $0 instance metadata path set contains duplicate UUIDs",
+ instance->path()),
+ JoinStrings(path_set.all_uuids(), ","));
}
- // Check that every all_uuids is the same.
- if (first_all_uuids.first.empty()) {
- first_all_uuids.first = all_uuids_str;
- first_all_uuids.second = instance;
- } else if (first_all_uuids.first != all_uuids_str) {
- return Status::IOError(Substitute(
- "File $0 claimed all_uuids $1 but file $2 claimed all_uuids $3",
- instance->filename_, all_uuids_str,
- first_all_uuids.second->filename_, first_all_uuids.first));
+ // Check that the instance's UUID set matches the expected set.
+ if (deduplicated_uuids != all_uuids) {
+ return Status::IOError(
+ Substitute("Data directories $0 and $1 have different instance metadata UUID sets",
+ instances[0]->path(), instance->path()),
+ Substitute("$0 vs $1", JoinStrings(all_uuids, ","), all_uuids_str));
}
}