You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2017/05/31 18:19:15 UTC
[kudu-CR] disk failure: make DataDirManager failure-aware
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7028
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that correspond
to failed directories.
Tests are added to data_dirs-test to ensure that failed directories are
not used and are not returned as part of newly created DataDirGroups. If
no healthy directories exist, callers will return an IOError with posix
code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
3 files changed, 126 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/1
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7028/15/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS15, Line 556: 0directory
nit: missing a space?
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#2).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that correspond
to failed directories. Additionally, a count of the number of known
failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories are
not used and are not returned as part of newly created DataDirGroups. If
no healthy directories exist, callers will return an IOError with posix
code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
3 files changed, 160 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/2
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7028/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS4, Line 480: LOG(WARNING) << Substitute("DataDirGroup for tablet $0 is smaller than targeted. "
: "Target size: $1, Actual size: $2", tablet_id,
: FLAGS_fs_target_data_dirs_per_tablet, group_indices.size());
can you add more info? like total num data dirs and total num failed data dirs?
PS4, Line 509: if (PREDICT_FALSE(!failed_data_dirs_.empty())) {
: for (uint16_t uuid_index : group->uuid_indices()) {
: if (!ContainsKey(failed_data_dirs_, uuid_index)) {
: valid_uuid_indices.emplace_back(uuid_index);
: }
: }
: if (valid_uuid_indices.empty()) {
: return Status::IOError("No healthy directories exist in tablet's "
: "DataDirGroup", opts.tablet_id, ENODEV);
: }
: group_uuid_indices = &valid_uuid_indices;
how about creating a helper method to filter failed dirs from a group of uuid_indices , can potentially use that in other places
PS4, Line 503: // Get the data dir group for the tablet.
: DataDirGroup* group = FindOrNull(group_by_tablet_map_, opts.tablet_id);
: if (group == nullptr) {
: return Status::NotFound("Tried to get directory but no DataDirGroup "
: "registered for tablet", opts.tablet_id);
: }
: if (PREDICT_FALSE(!failed_data_dirs_.empty())) {
: for (uint16_t uuid_index : group->uuid_indices()) {
: if (!ContainsKey(failed_data_dirs_, uuid_index)) {
: valid_uuid_indices.emplace_back(uuid_index);
: }
: }
: if (valid_uuid_indices.empty()) {
: return Status::IOError("No healthy directories exist in tablet's "
: "DataDirGroup", opts.tablet_id, ENODEV);
: }
: group_uuid_indices = &valid_uuid_indices;
: } else {
: group_uuid_indices = &group->uuid_indices();
: }
not from this patch but can you revert the if to put the non-test logic first?
PS4, Line 573: // TODO(awong): disk fullness and, to an extent, disk failure, are
: // transient states. If a disk is in either state at the time of group
: // creation, the resulting group may be below targeted size. Add
: // functionality to resize groups.
the whole management of data dir lifecycles (retiring failed data dirs or fixing them, adding new ones etc) likely merits a jira and not just a comment
http://gerrit.cloudera.org:8080/#/c/7028/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS4, Line 258: const std::vector<std::unique_ptr<DataDir>>& data_dirs() const {
: return data_dirs_;
: }
can you add docs stating whether this returns healthy/failed/all data dirs?
PS4, Line 264: std::set<std::string> FindTabletsByDataDirUuidIdx(uint16_t uuid_idx);
maybe refactor this to return a status and pass the set by pointer as an out param? if you plan to ever allow an admin to (hot) remove a data dir, might be useful to be able to return something like NotFound here is the uuid_idx doesn't exist. If this is not a concern, or at least not something that we'll want to do online, never mind
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#8).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 194 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/8
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#10).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 198 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/10
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#14).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 210 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/14
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 1:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
Line 86: CreateBlockOptions non_existent_tablet_opts(test_block_opts_);
Why do we need to copy test_block_opts_? Can't we just pass it directly to GetNextDataDir()?
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 443: shared_lock<rw_spinlock> health_lock(dir_health_lock_.get_lock());
This is why I asked about the contention profiles of the locks: use and acquisition of multiple locks should only be done if absolutely necessary, because it's really easy to make a mistake and cause a deadlock via lock inversion.
PS1, Line 460: group_target_size = std::min(group_target_size,
: static_cast<uint32_t>(data_dirs_.size() - failed_data_dirs_.size()));
If someone requested a group of a particular size, why not fail that request if there have been too many failures? Why be permissive?
PS1, Line 484: // This should only be reached by some tests; in cases where there is no
: // natural tablet_id, select a data dir from any of the directories.
So in this case there's no point in considering failed_data_dirs_?
Line 503: return Status::IOError("No healthy directories exist in tablet's "
No ENODEV here?
Line 587: bool DataDirManager::FindTabletsByDataDirUuidIdx(uint16_t uuid_idx, set<string>** tablet_ids) {
This needs some locking.
Line 601: LOG(ERROR) << Substitute("Dir with uuid index $0 ($1) marked as failed", uuid_idx, dd->dir());
Perhaps avoid logging this multiple times if failed_data_dirs_ already has this uuid_idx?
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 263: bool FindTabletsByDataDirUuidIdx(uint16_t uuid_idx, std::set<std::string>**
I imagine you're trying to avoid copies by passing the caller a pointer into the map, but this makes synchronization and lifecycle management much harder. To simplify, just have tablet_ids be a pointer and copy the set's contents.
Also, I don't understand the difference between returning false and returning true with an empty set. It seems to me that uuid_idx is guaranteed to be in the map; only the contents of tablet_ids can vary (from empty to non-empty). So you could return the set directly instead of passing it as an argument.
PS1, Line 268: MarkDataDirFailure
Nit: how about MarkDataDirFailed, for better symmetry with IsDirectoryFailed?
Line 271: bool IsDirectoryFailed(uint16_t uuid_idx) const;
Nit: Likewise, IsDataDirFailed (or HasDataDirFailed)
Line 273: void GetFailedDataDirs(std::set<uint16_t>* data_dirs) const {
How about just returning the data_dirs? Easier for callers.
Line 274: *data_dirs = failed_data_dirs_;
This needs to acquire a lock, no?
Line 329: // Lock protecting changes to failed_data_dirs_.
Why do we need a different lock for this? Do you expect either this lock or dir_group_lock_ to be highly contended?
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 13:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7028/13/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS13, Line 58: DEFINE_int32(fs_target_data_dirs_per_tablet, 0,
: "Indicates the target number of data dirs to spread each "
: "tablet's data across. If greater than the number of data dirs "
: "available, data will be striped across those available. The "
: "default value 0 indicates striping should occur across all "
: "data directories.");
: DEFINE_validator(fs_target_data_dirs_per_tablet,
: [](const char* /*n*/, int32_t v) { return v >= -1; });
> if -1 is a valid value, can you document what it means?
-1 shouldn't be valid.
Line 488: DCHECK_GE(group_target_size, 0);
> this assertion probably belongs one line up, right? otherwise it's a no-op
Ah you're right, my mistake.
PS13, Line 497: DataDirGroup for tablet $0 is smaller than targeted
> for a user-facing message, can we make this a bit clearer and with less exp
Done
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 13:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7028/13/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS13, Line 58: DEFINE_int32(fs_target_data_dirs_per_tablet, 0,
: "Indicates the target number of data dirs to spread each "
: "tablet's data across. If greater than the number of data dirs "
: "available, data will be striped across those available. The "
: "default value 0 indicates striping should occur across all "
: "data directories.");
: DEFINE_validator(fs_target_data_dirs_per_tablet,
: [](const char* /*n*/, int32_t v) { return v >= -1; });
if -1 is a valid value, can you document what it means?
Line 488: DCHECK_GE(group_target_size, 0);
this assertion probably belongs one line up, right? otherwise it's a no-op
PS13, Line 497: DataDirGroup for tablet $0 is smaller than targeted
for a user-facing message, can we make this a bit clearer and with less exposure of implementation details like class names? eg something like:
Only able to allocate <N> of requested <M> directories for tablet <T>. (<A> dirs total, <B> full, <C> failed)
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 1:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
Line 86: CreateBlockOptions non_existent_tablet_opts(test_block_opts_);
> Why do we need to copy test_block_opts_? Can't we just pass it directly to
Was meant for explicitness, but it should be obvious given the comment.
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 443: shared_lock<rw_spinlock> health_lock(dir_health_lock_.get_lock());
> This is why I asked about the contention profiles of the locks: use and acq
Fair, it is messy and risky, adding another lock. I thought proper ordering would be sufficient to prevent contention, but good point that there's not much gained and a big potential headache.
PS1, Line 460: group_target_size = std::min(group_target_size,
: static_cast<uint32_t>(data_dirs_.size() - failed_data_dirs_.size()));
> If someone requested a group of a particular size, why not fail that reques
As discussed, there may be cases where we want either behavior. If the user just wants to get things done without regard for the performance hit, we may want to understripe the tablet. If the user cares very much about performance, "overstriping" (by assigning full or failed disks) may be the way to go.
As you mentioned, in the case of understriping, if it is important to a user, we'll need to add functionality to resize a directory group. Left this as a TODO.
PS1, Line 484: // This should only be reached by some tests; in cases where there is no
: // natural tablet_id, select a data dir from any of the directories.
> So in this case there's no point in considering failed_data_dirs_?
I don't think so; this isn't used by all tests, and I would expect tests that do expect disk failures to not use this codepath and to specify a tablet.
Line 503: return Status::IOError("No healthy directories exist in tablet's "
> No ENODEV here?
Done
Line 587: bool DataDirManager::FindTabletsByDataDirUuidIdx(uint16_t uuid_idx, set<string>** tablet_ids) {
> This needs some locking.
Done
Line 601: LOG(ERROR) << Substitute("Dir with uuid index $0 ($1) marked as failed", uuid_idx, dd->dir());
> Perhaps avoid logging this multiple times if failed_data_dirs_ already has
Done
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
Line 263: bool FindTabletsByDataDirUuidIdx(uint16_t uuid_idx, std::set<std::string>**
same line
Line 263: bool FindTabletsByDataDirUuidIdx(uint16_t uuid_idx, std::set<std::string>**
> I imagine you're trying to avoid copies by passing the caller a pointer int
Hrm good point, you're right that I was trying to avoid copies, but agreed, that's probably not worth the potential synchronization headache. If left as is, I think external locking would be necessary.
Using both suggestions.
PS1, Line 268: MarkDataDirFailure
> Nit: how about MarkDataDirFailed, for better symmetry with IsDirectoryFaile
Done
Line 271: bool IsDirectoryFailed(uint16_t uuid_idx) const;
> Nit: Likewise, IsDataDirFailed (or HasDataDirFailed)
Done
Line 273: void GetFailedDataDirs(std::set<uint16_t>* data_dirs) const {
> How about just returning the data_dirs? Easier for callers.
Done
Line 274: *data_dirs = failed_data_dirs_;
> This needs to acquire a lock, no?
Done
Line 329: // Lock protecting changes to failed_data_dirs_.
> Why do we need a different lock for this? Do you expect either this lock or
Noted in another comment that this is a good point. Removed this lock and am just using dir_group_lock_.
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#12).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 203 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/12
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#9).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 198 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/9
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 15: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7028/15/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS15, Line 556: 0directory
> nit: missing a space?
oops, andrew pointed out that the string already has a space
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 587: candidate_indices.erase(candidate_indices.begin());
> This needs some locking.
Good point, although I think the lock may need to be external.
http://gerrit.cloudera.org:8080/#/c/7028/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS4, Line 480: LOG(WARNING) << Substitute("DataDirGroup for tablet $0 is smaller than targeted. "
: "Target size: $1, Actual size: $2", tablet_id,
: FLAGS_fs_target_data_dirs_per_tablet, group_indices.size());
> can you add more info? like total num data dirs and total num failed data d
Done
PS4, Line 509: if (PREDICT_FALSE(!failed_data_dirs_.empty())) {
: for (uint16_t uuid_index : group->uuid_indices()) {
: if (!ContainsKey(failed_data_dirs_, uuid_index)) {
: valid_uuid_indices.emplace_back(uuid_index);
: }
: }
: if (valid_uuid_indices.empty()) {
: return Status::IOError("No healthy directories exist in tablet's "
: "DataDirGroup", opts.tablet_id, ENODEV);
: }
: group_uuid_indices = &valid_uuid_indices;
> how about creating a helper method to filter failed dirs from a group of uu
Done, open to API suggestions, for now I have it return a bool indicating whether there are failed data dirs with a pointer input (to avoid an extra allocation).
PS4, Line 503: // Get the data dir group for the tablet.
: DataDirGroup* group = FindOrNull(group_by_tablet_map_, opts.tablet_id);
: if (group == nullptr) {
: return Status::NotFound("Tried to get directory but no DataDirGroup "
: "registered for tablet", opts.tablet_id);
: }
: if (PREDICT_FALSE(!failed_data_dirs_.empty())) {
: for (uint16_t uuid_index : group->uuid_indices()) {
: if (!ContainsKey(failed_data_dirs_, uuid_index)) {
: valid_uuid_indices.emplace_back(uuid_index);
: }
: }
: if (valid_uuid_indices.empty()) {
: return Status::IOError("No healthy directories exist in tablet's "
: "DataDirGroup", opts.tablet_id, ENODEV);
: }
: group_uuid_indices = &valid_uuid_indices;
: } else {
: group_uuid_indices = &group->uuid_indices();
: }
> not from this patch but can you revert the if to put the non-test logic fir
Done
PS4, Line 573: // TODO(awong): disk fullness and, to an extent, disk failure, are
: // transient states. If a disk is in either state at the time of group
: // creation, the resulting group may be below targeted size. Add
: // functionality to resize groups.
> the whole management of data dir lifecycles (retiring failed data dirs or f
Done
http://gerrit.cloudera.org:8080/#/c/7028/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS4, Line 258: const std::vector<std::unique_ptr<DataDir>>& data_dirs() const {
: return data_dirs_;
: }
> can you add docs stating whether this returns healthy/failed/all data dirs?
Done
PS4, Line 264: std::set<std::string> FindTabletsByDataDirUuidIdx(uint16_t uuid_idx);
> maybe refactor this to return a status and pass the set by pointer as an ou
I agree that when that happens, this will need to change, but since that isn't yet a feature, and since the status won't be needed in this patch, I think we should defer this until recovery is implemented.
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7028/11/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS11, Line 471: ainsKey(group_by_tablet_map_, tabl
> maybe put the cast around size() instead of the result of the subtraction?
Done
PS11, Line 622: shared_lock<rw_spinlock> lock(
> missing a '!'?
Done
Line 632: std::lock_guard<percpu_rwlock> lock(dir_group_lock_);
> can we DCHECK_LT(uuid_idx, data_dirs_.size()) or something? especially with
Done
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7028/2/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
Line 88: METRIC_DEFINE_gauge_uint64(server, data_dirs_failed,
Nit: "failed" sorts ahead of "full" (update elsewhere if needed).
PS2, Line 610: set<string>()
Can you return {} instead?
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7028/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS1, Line 460: group_target_size = std::min(group_target_size,
: static_cast<uint32_t>(data_dirs_.size() - failed_data_dirs_.size()));
> As discussed, there may be cases where we want either behavior. If the user
I'd like to think about this a bit more and revisit/discuss it within the next couple of days, particularly failures vs fullness.
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#5).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
3 files changed, 186 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/5
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Reviewed-on: http://gerrit.cloudera.org:8080/7028
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 221 insertions(+), 41 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
PS6, Line 222: are not the in the failed
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS6, Line 459: uint32_t group_target_size
> would this be simpler to just be an 'int'? GSG discourages using uints for
I don't think it makes things simpler since a cast is still needed for the *.size() call, but I'll switch out uint32_t for uniformity with GSG.
PS6, Line 632: LOG(ERROR) << Substitute("Dir with uuid index $0 ($1) marked as failed", uuid_idx, dd->dir());
> Would it be convenient to take a Status or a string message as an argument
Done
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 268: served to or from
> nit: reads a bit funny
Done
PS6, Line 280: // Returns true if all data dirs registered with the DataDirManager are healthy.
: // If there are failed data dirs, populates 'healthy_indices' with those in
: // 'uuid_indices' that have not failed and returns false.
> hrm, this is a sort of odd contract... if I understand correctly, you're sa
In that case, it would return false and use use the outputted healthy_indices.
I'd wanted to piggy-back this function to also be a check emptiness of failed_data_dirs_. A cleaner API is probably more important here since the (has failed data dirs) branch should ideally not be used much anyway.
Line 304: // lock_guard of both dir_group_lock_ and dir_health_lock_.
> I don't see dir_health_lock_. Is this left from a previous iteration of the
Yes and done.
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#3).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that correspond
to failed directories. Additionally, a count of the number of known
failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories are
not used and are not returned as part of newly created DataDirGroups. If
no healthy directories exist, callers will return an IOError with posix
code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
3 files changed, 161 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/3
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7028/11/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS11, Line 471: static_cast<int>(data_dirs_.size()
maybe put the cast around size() instead of the result of the subtraction? Otherwise this unsigned subtraction could underflow?
PS11, Line 622: if (error_message.empty()) {
missing a '!'?
Line 632: return ContainsKey(failed_data_dirs_, uuid_idx);
can we DCHECK_LT(uuid_idx, data_dirs_.size()) or something? especially with these unsigned ints floating around I think it's worth trying to guard against underflows
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 3: Code-Review+1
Todd and/or David should also take a look.
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs-test.cc
File src/kudu/fs/data_dirs-test.cc:
PS6, Line 222: are not the in the failed
typo
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
PS6, Line 459: uint32_t group_target_size
would this be simpler to just be an 'int'? GSG discourages using uints for things that we expect to just be small integer counts
PS6, Line 632: LOG(ERROR) << Substitute("Dir with uuid index $0 ($1) marked as failed", uuid_idx, dd->dir());
Would it be convenient to take a Status or a string message as an argument to this function that could be used to improve this log message? Would be nice if, when we saw this, we could see some indication of the underlying issue which caused it to be marked failed (eg out of space, IO error, readonly FS, etc)
http://gerrit.cloudera.org:8080/#/c/7028/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
PS6, Line 268: served to or from
nit: reads a bit funny
PS6, Line 280: // Returns true if all data dirs registered with the DataDirManager are healthy.
: // If there are failed data dirs, populates 'healthy_indices' with those in
: // 'uuid_indices' that have not failed and returns false.
hrm, this is a sort of odd contract... if I understand correctly, you're saying that if all are healthy, then healthy_indices won't be touched, but if some are unhealthy, then healthy_indices will contain the healthy ones?
What if there is an unhealthy disk that is not listed in 'uuid_indices'? Would it return true or false?
Is there a simpler API that could be used here, like: void RemoveUnhealthyDirs(vector<uint16_t>* uuid_indices)?
Line 304: // lock_guard of both dir_group_lock_ and dir_health_lock_.
I don't see dir_health_lock_. Is this left from a previous iteration of the patch?
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] disk failure: make DataDirManager failure-aware
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7028
to look at the new patch set (#15).
Change subject: disk failure: make DataDirManager failure-aware
......................................................................
disk failure: make DataDirManager failure-aware
The DataDirManager must record what directories are unhealthy in order
to avoid placing new data on failed disks. This patch achieves this by
maintaining a set of UUID indices in the DataDirManager that
correspond to failed directories. Additionally, a count of the number
of known failed directories is maintained as a metric.
Tests are added to data_dirs-test to ensure that failed directories
are not used and are not returned as part of newly created
DataDirGroups. If no healthy directories exist, callers will return an
IOError with posix code ENODEV.
Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
4 files changed, 221 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/15
--
To view, visit http://gerrit.cloudera.org:8080/7028
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>