You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org> on 2021/07/23 18:55:00 UTC
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17725
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
4 files changed, 50 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/1
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: ble_space_cache_seconds);
> Yeah, another approach would be to make this a member function, like Refres
Yes, having a member function makes reduces complications in the code.
http://gerrit.cloudera.org:8080/#/c/17725/12/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/12/src/kudu/server/server_base.cc@962
PS12, Line 962: ;
> nit: an extra semicolon
Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Aug 2021 19:20:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 4:
(2 comments)
Will address the rest of the pointed out issues in the next patch
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@437
PS4, Line 437: FLAGS_fs_data_dirs_available_space_cache_seconds
> Not sure if we want to use a separate flag for wal dir caching?
As discussed with Andrew offline will add a new flag.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@443
PS4, Line 443: wal_dir_available_space = -1;
: return wal_dir_available_space;
> As a user, I'm curious if it would make sense to leave the value as is on f
Yes, that makes sense. Will try to ensure:
1. For WAL dir failure initialize it to -1 and return the last known value in case of disk failure after the TS started
2. For data dir initialize it to -1 and skip the calculation of disk space whose statvfs fails.
So in case of complete initial failure of wal or all data disks, -1 will be returned
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Aug 2021 02:30:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/17725/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/3//COMMIT_MSG@9
PS3, Line 9: This commit adds two metrics to server entity.
: wal_dir_space_available_bytes - Exposes the free space available in the
: wal directory
: data_dirs_space_available_bytes - Exposes the sum of free space in each
: of the data directories.
nit: may be more readable as
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes: Exposes the free space available in the
WAL directory.
- data_dirs_space_available_bytes: Exposes the sum of free space in each
of the data directories.
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc@101
PS3, Line 101: DEFINE_int64(fs_wal_dir_reserved_bytes, -1,
Flags should only be defined once, so we should remove the definition in the consensus module.
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@258
PS3, Line 258: wal
nit: capitalized WAL here and below
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@431
PS3, Line 431: con
nit: add a space
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@431
PS3, Line 431: int64_t GetWalDirAvailableSpace(const ServerBaseOptions options,const FsManager& fs_manager) {
We should pass the options as a const reference, otherwise we'll be making a copy of the options every time this is called.
Same below.
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@434
PS3, Line 434: if (!options.env->GetSpaceInfo(fs_manager.GetWalsRootDir(), &space_info_waldir).ok()) {
: return -1;
: }
This seems a bit surprising to see as a metric. I'm curious, as a user whether it would be more useful to cache the current state of this metric and return a stale value if this fails. If we do that, we could also introduce some form of rate limiting a la --fs_data_dirs_available_space_cache_seconds, so we're not exposing a system call every time we want to get this metric (even if we only expect it to infrequent).
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@452
PS3, Line 452: unique_fsid
nit: unique is implied because this is a set. Maybe just call this 'fs_ids'?
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@453
PS3, Line 453: const string &
nit: put the ampersand next to "string", i.e. "const string& data_dir", or better yet, "const auto& data_dir"
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@457
PS3, Line 457: (FLAGS_fs_data_dirs_reserved_bytes == -1)
nit: we could define this once up front as a bool as some
bool should_reserve_one_percent = FLAGS_fs_data_dirs_reserved_bytes == -1;
That also has the benefit of being somewhat self-explanatory.
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@460
PS3, Line 460: if (space_info_datadir.free_bytes > reserved_bytes) {
: total_free_space += space_info_datadir.free_bytes - reserved_bytes;
: }
nit: would be simpler as
total_free_space += max(0, space_info_datadir.free_bytes - reserved_bytes);
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/server/server_base.cc@926
PS3, Line 926: [this]() {return GetDataDirAvailableSpace(options_, *fs_manager_.get());})->
> warning: redundant get() call on smart pointer [readability-redundant-smart
Right, you can just do *fs_manager_ to pass as a reference -- that's true for all smart pointer types.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 04:09:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 13:
(2 comments)
Seems lint is also unhappy with some line lengths.
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc@953
PS13, Line 953: MonoTime expiry = wal_dir_space_last_check_ + MonoDelta::FromSeconds(
: FLAGS_fs_wal_dir_available_space_cache_seconds);
: if (MonoTime::Now() > expiry) {
: wal_dir_space_last_check_ = MonoTime::Now();
: SpaceInfo space_info_waldir;
: wal_dir_available_space_ = CalculateAvailableSpace(options, fs_manager.GetWalsRootDir(),
: FLAGS_fs_wal_dir_reserved_bytes,
: &space_info_waldir);
: }
: }
I have a concern about thread safety that was actually true with the previous iterations. It seems like we should probably hold a lock whenever we want to read or write to the available space members.
Consider adding a simple_spinlock, and taking the lock only to update (not to calculate, given calculation may be expensive) the values. E.g.
auto now = MonoTime::Now();
if (MonoTime::Now() > expiry) {
SpaceInfo space_info_waldir;
auto wal_dir_space = CalculateAvailableSpace(options, fs_manager.GetWalsRootDir(),
FLAGS_fs_wal_dir_reserved_bytes,
&space_info_waldir);
std::lock_guard<simple_spinlock> l(lock_);
wal_dir_space_last_check_ = now;
wal_dir_available_space_ = wal_dir_space;
}
Same with data dirs.
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc@972
PS13, Line 972: data_dirs_available_space_ = 0;
We shouldn't be setting this to 0 every time we call this function, right?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 13
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Aug 2021 20:27:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 10:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@22
PS10, Line 22: mac
> nit: macOS ?
Done
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@24
PS10, Line 24: directores
> nit: directories
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc@84
PS10, Line 84: fs_wal_dir_available_space_cache_seconds
> Is it intentional that setting this flag to a negative value leads to alway
Done. Applied the validator for --fs_data_dirs_available_space_cache_seconds (above) too.
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@352
PS10, Line 352: E
> nit: got a couple extra spaces here
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@364
PS10, Line 364: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value",
: &wal_dir_space));
: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value",
: &data_dir_space));
: ASSERT_NE(wal_dir_space, -1);
> nit: we can use get_metrics() here too, right?
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@260
PS10, Line 260: WAL directory space free"
> nit: usually, the metric's label hash all words started with capital letter
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@262
PS10, Line 262: Returns "
: "-1 if reading the disk fails
> nit: maybe rephrase as
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@266
PS10, Line 266: "data directories space free"
> nit: "Data Directories Free Space"
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@268
PS10, Line 268: Returns "
: "-1 if reading any of the disks fails
> nit:
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@438
PS10, Line 438: int64_t reserved_bytes;
: bool should_reserve_one_percent;
> nit: move these down where they assigned with their values, turning these d
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@446
PS10, Line 446: static_cast<int64_t> (0)
> nit: could replace with 0LL ?
Yes, it was 0LL in patchset 4. But I remember one of the test configurations (maybe ASAN or TSAN) was not happy about it, hence changed it to the above. Do not have the old logs anymore when checked. I'll resubmit the next patch with 0LL and we can check it.
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: int64_t* wal_dir_available_space
> Why not to return this from this function? Why to keep this as an output p
Yes, that makes sense. Done.
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@474
PS10, Line 474:
> indentation nit: remove one space here and for two lines below
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@476
PS10, Line 476: int64_t* data_dirs_available_space
> Why to have this as an output parameter? Maybe, just return the calculated
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@479
PS10, Line 479: SpaceInfo space_info_datadir;
> nit: move this under the 'if()' scope below?
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@483
PS10, Line 483: int64_t available_space;
> nit: move this under the scope of the 'for()' cycle below?
Done
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@494
PS10, Line 494: data_dirs_available_space
> What if data_dirs_available_space points to non-zeroed memory? Is it inten
We make data_dirs_available_space to point to zero in line 484 just before the 'for()'
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h@48
PS10, Line 48: q:q
> nit: drop these trailing symbols?
Oops. Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Aug 2021 15:26:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#12).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 180 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/12
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 12: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@446
PS10, Line 446:
> Reverting this to static_cast<int64_t> as we see the below issue:
Ah, I see -- makes sense. I didn't track this back to patchset4, sorry.
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: ble_space_cache_seconds);
> Changing back to out parameter in the next patch (patchset 12).
This function is used only in one place, as I can see. Moving the logic into the metric functor completely might help to get rid of all these in-out and out parameters and those rules of populating these. But I don't feel strongly about this, was a bit surprised to see such a signature for a function, though.
It's OK with me to keep it as is, though.
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: ble_space_cache_seconds);
> This doesn't return anything when we're passed our expiration. Having this
Ah, that makes sense, but it's bit tricky anyway -- this function is used in just one place and has in-out and an out parameter. Let's keep it as is, maybe.
http://gerrit.cloudera.org:8080/#/c/17725/12/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/12/src/kudu/server/server_base.cc@962
PS12, Line 962: ;
nit: an extra semicolon
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Aug 2021 21:57:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 12: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: ble_space_cache_seconds);
> This function is used only in one place, as I can see. Moving the logic in
Yeah, another approach would be to make this a member function, like RefreshWalDirAvailableSpaceIfExpired(), and having the docs mentioning it only updates the values if expired.
That said, I'm also OK with keeping this as is
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 24 Aug 2021 21:14:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#3).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
5 files changed, 77 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/3
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 5:
(11 comments)
Mostly nits here. Looking pretty good otherwise!
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@80
PS5, Line 80:
nit: not related to this change, but there's an extra space at the end here
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@85
PS5, Line 85: the block manager.
nit: WAL directory
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336
PS5, Line 336:
nit: remove the extra space. Same at L358
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336
PS5, Line 336: ASSERT_NE(wal_dir_space, -1 );
: ASSERT_NE(data_dir_space, -1);
Could we instead ASSERT_GT(wal_dir_space, 0)? Same with data_dir_space. Almost surely the test machines will have space. Same at L348?
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@349
PS5, Line 349: LOG(INFO) << "Restarting server with injected errors...";
nit: this is useful while iterating on a test, but before merging, let's remove the extra log, given if we needed to debug a test failure, it should be clear when we restart the server without this line.
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@262
PS5, Line 262: +
nit: remove the "+"? Same at L268
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@437
PS5, Line 437: int64
nit: int64_t?
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@468
PS5, Line 468: data_dirs_available_space
nit: wrap variable names with single quotes so it's easier upon reading to immediately tell that this is referring to a variable. Same elsewhere.
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@472
PS5, Line 472: fs_data_dirs_available_space_cache_seconds
nit: prefix with -- (i.e. --fs_data_di...)
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@961
PS5, Line 961: [this]() {GetDataDirAvailableSpace(options_, *fs_manager_,
: &data_dirs_space_last_check_, &data_dirs_available_space_); return data_dirs_available_space_;})->
: AutoDetachToLastValue(&metric_detacher_);
style nit: typically we style lambdas (especially multi-line ones) as we would regular functions.
METRIC_...InstantiateFunctionGauge(
metric_entity_,
[this]() {
GetDataDirAvailableSpace(options_, *fs_manager_,
&data_dirs_space_last_check_, &data_dirs_available_space_);
return data_dirs_available_space_;
})
->AutoDetachToLastValue(&metric_detatcher_);
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc@1446
PS5, Line 1446: space_info->filesystem_id = buf.f_fsid;
I'm curious if you've tried this on a real machine with multiple mounted devices. In tests it's a bit difficult to check that, but it's probably worth manually verifying. E.g. maybe try mounting two data directories in one volume, and one data directory in a separate one. We should see that the available space should be the sum available in the two volumes (no more, no less).
If you do end up performing that manual test, be sure to mention what you did in the commit message.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 13 Aug 2021 01:04:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 5:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@80
PS5, Line 80:
> nit: not related to this change, but there's an extra space at the end here
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/fs/data_dirs.cc@85
PS5, Line 85: the block manager.
> nit: WAL directory
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336
PS5, Line 336:
> nit: remove the extra space. Same at L358
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@336
PS5, Line 336: ASSERT_NE(wal_dir_space, -1 );
: ASSERT_NE(data_dir_space, -1);
> Could we instead ASSERT_GT(wal_dir_space, 0)? Same with data_dir_space. Alm
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/integration-tests/disk_failure-itest.cc@349
PS5, Line 349: LOG(INFO) << "Restarting server with injected errors...";
> nit: this is useful while iterating on a test, but before merging, let's re
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@262
PS5, Line 262: +
> nit: remove the "+"? Same at L268
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@437
PS5, Line 437: int64
> nit: int64_t?
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@468
PS5, Line 468: data_dirs_available_space
> nit: wrap variable names with single quotes so it's easier upon reading to
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@472
PS5, Line 472: fs_data_dirs_available_space_cache_seconds
> nit: prefix with -- (i.e. --fs_data_di...)
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/server/server_base.cc@961
PS5, Line 961: [this]() {GetDataDirAvailableSpace(options_, *fs_manager_,
: &data_dirs_space_last_check_, &data_dirs_available_space_); return data_dirs_available_space_;})->
: AutoDetachToLastValue(&metric_detacher_);
> style nit: typically we style lambdas (especially multi-line ones) as we wo
Done
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/17725/5/src/kudu/util/env_posix.cc@1446
PS5, Line 1446: space_info->filesystem_id = buf.f_fsid;
> I'm curious if you've tried this on a real machine with multiple mounted de
Yes, tested that in my local mac machine earlier by adding a new volume.
Snippet of df -h:
Filesystem 512-blocks Used Available Capacity iused ifree %iused Mounted on
/dev/disk1s1 976490576 21607536 654724040 4% 488003 4881964877 0% /
/dev/disk1s6 466Gi 696Ki 312Gi 1% 72 4882452808 0% /Volumes/Test
In both the below data directories the space usage reported is identical:
Test 1:
/Users/achennaka/master-0/data,/Volumes/Test/kudu,/Volumes/Test/kudu1
Test 2:
/Users/achennaka/master-0/data,/Volumes/Test/kudu
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 14 Aug 2021 06:22:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#6).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local mac machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directores on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the the
flag fs_data_dirs_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 164 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/6
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#7).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local mac machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directores on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the the
flag fs_data_dirs_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 172 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/7
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#9).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local mac machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directores on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 171 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/9
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc@101
PS3, Line 101: DEFINE_int64(fs_wal_dir_reserved_bytes, -1,
> Flags should only be defined once, so we should remove the definition in th
That's done already. The definition is removed from consensus/log.cc
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: // Calculates the total free space on all the data directories combined accounting for
> Yes. It could be inlined too if you want to avoid the overhead of calling a
Thanks, I am trying to understand the benefit of having a separate function.
if (!options.env->GetSpaceInfo(data_dir, &space_info_datadir).ok()) {
return -1;
}
will be replaced by something like:
Status s = GetDirSpace(options, data_dir, space_info_datadir)
if (!s.ok()) {
return -1;
}
And GetDirSpace function will be calling GetSpaceInfo. Is my understanding correct?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Aug 2021 14:58:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#11).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 174 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/11
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@446
PS10, Line 446:
> Yes, it was 0LL in patchset 4. But I remember one of the test configuration
Reverting this to static_cast<int64_t> as we see the below issue:
11:36:13 /home/jenkins-slave/workspace/kudu-master/0/src/kudu/server/server_base.cc:444:63: error: no matching function for call to 'max(long long int, int64_t)'
11:36:13 return std::max(0LL, space_info->free_bytes - reserved_bytes);
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: y) {
> Ah, right thanks for pointing that out. Knew there was a reason for that bu
Changing back to out parameter in the next patch (patchset 12).
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Aug 2021 21:28:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt
File src/kudu/server/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt@62
PS2, Line 62: consensus
I'm not sure I understand why this is now required here: server_process is supposed to be free of consensus-specific stuff, so this looks a bit off to me. Could you clarify?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@430
PS2, Line 430: Calculates the free space on the WAL disk
nit: add a period in the end of the sentence
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@431
PS2, Line 431: unique_ptr<FsManager> &fs_manager
I'm not sure it makes sense to pass unique_ptr by a reference: why not to pass just a pointer to FsManager* instead or even a reference to FsManager since GetWalsRootDir() is supposed to be a constant method?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@431
PS2, Line 431: ServerBaseOptions options
Why not to pass ServerBaseOptions as a const reference?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@434
PS2, Line 434: space_info_waldir.free_bytes - FLAGS_fs_wal_dir_reserved_bytes;
There might be dragons here: what if this turns to be a negative number? What would this function then return given the return type is uint64_t?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@438
PS2, Line 438: // multiple directories on a single disk
nit: add a period in the end of the sentence
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@439
PS2, Line 439: const unique_ptr<FsManager> &fs_manager
Why not to use just 'const FsManager&' here?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: FLAGS_fs_data_dirs_reserved_bytes
What if this is set to the default value of `-1`?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: total_free_space +=
There might be dragons here: what if (space_info_datadir.free_bytes - FLAGS_fs_data_dirs_reserved_bytes) turns to be negative and abs(total_free_space) < abs(space_info_datadir.free_bytes - FLAGS_fs_data_dirs_reserved_bytes) ?
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@447
PS2, Line 447: unique_fsid.insert(space_info_datadir.filesystem_id);
Maybe, instead of this check-and-then-insert pattern, use simply try-insert-and-check-whether-inserted to avoid double-lookup in the dictionary. For that, it's possible to use InsertIfNotPresent() or alike.
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/util/env.h@48
PS2, Line 48:
nit: it would be nice to align this comment with two comment above
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/util/env_posix.cc@1447
PS2, Line 1447:
nit: the indent is off
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 26 Jul 2021 18:38:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 18 Aug 2021 17:30:52 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt
File src/kudu/server/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt@62
PS2, Line 62: consensus
> Could we move the definition of the fs_wal_dir_reserved_bytes to, e.g. fs_m
+1 to the approach suggested by Andrew.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 27 Jul 2021 23:40:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@325
PS9, Line 325: We could do ASSERT_EQ of the metrics and the current available
: // space but there will most likely be some data written between the two calls causing
: // a failure.
> Sure we can't check that they're equal to each other, but could we check th
Done
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@330
PS9, Line 330: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value",
: &wal_dir_space));
: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value",
: &data_dir_space));
:
> nit: Given we're using this in a few places now (and maybe one more to come
Done
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@346
PS9, Line 346: Substitute(
> nit: there are no substitutions happening, so we can remove this.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Aug 2021 19:07:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 5:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: return std::max(static_cast<int64_t> (0), space_info.free_bytes - reserved_bytes);
> Not exactly, I meant wrapping the calculation too, to make sure they're cal
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@259
PS4, Line 259: erv
> nit: WAL. Same elsewhere
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@266
PS4, Line 266: "data directories space free",
> We should also mention the error behavior that this returns -1
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@431
PS4, Line 431: return shared_ptr<MemTracker>(MemTracker::C
> nit: could you also document the error semantics? Same with the other metho
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@432
PS4, Line 432: }
> nit: this doesn't need to have a return value if values are being returned
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434
PS4, Line 434: WAL/data directory's di
> nit: we should keep the naming of our variables standardized as separating
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434
PS4, Line 434: the given WAL/data directory's disk. Returns -1 in case of disk
: // failure.
> nit: parameters that are output values (even if they're also input values)
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@437
PS4, Line 437: int64 flag_reserved_b
> As discussed with Andrew offline will add a new flag.
Done
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@443
PS4, Line 443: should_reserve_one_percent = flag_reserved_bytes == -1;
: reserved_bytes = should_reserve_one
> Yes, that makes sense. Will try to ensure:
As disucssed it is designed to:
1. For WAL dir failure initialize it to -1 and return -1 in case of disk failure after the TS started
2. For data dir initialize it to -1 and return -1 if any of the calls to statvfs fails.
So in case of complete initial failure of WAL or all data disks, -1 will be returned.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@447
PS4, Line 447: }
> should_preserve_one_percent is already a boolean. You don't need further co
Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 Aug 2021 18:58:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@919
PS14, Line 919: wal_dir_available_space_
> I was going to point out that we need to take the lock before returning thi
+1
It would be consistent to update the internals in Refresh{Data,Wal}DirAvailableSpaceIfExpired(), and return the result.
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@953
PS14, Line 953: MonoTime expiry = wal_dir_space_last_check_ + MonoDelta::FromSeconds(
: FLAGS_fs_wal_dir_available_space_cache_seconds);
> We also need to hold the lock here. Consider early returning when possible
Alternatively, it's possible to make wal_dir_space_last_check_ and wal_dir_available_space_ atomic. Then you could use compare_exchange to set wal_dir_space_last_check_ and wal_dir_available_space_, making sure only the very last update is kept :)
Same for data_dirs_space_last_check_ and data_dirs_available_space_.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:50:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: bool should_reserve_one_percent = FLAGS_fs_data_dirs_reserved_bytes == -1;
> Thanks, I am trying to understand the benefit of having a separate function
Not exactly, I meant wrapping the calculation too, to make sure they're calculated the same way and that we don't need to change it in two places.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Aug 2021 12:48:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 17
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Aug 2021 01:26:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#15).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 190 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/15
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 15
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#8).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local mac machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directores on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 172 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/8
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 4:
(10 comments)
I started reviewing this file, but realized there were some comments left unaddressed, so maybe this isn't ready for a re-review? In either case, I'll leave the comments I had this time around.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/consensus/log.cc@148
PS4, Line 148: DECLARE_int64(fs_data_dirs_reserved_bytes);
We shouldn't need to declare this here since it's not used in this file.
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17725/3/src/kudu/fs/fs_manager.cc@101
PS3, Line 101: DEFINE_int64(fs_wal_dir_reserved_bytes, -1,
> That's done already. The definition is removed from consensus/log.cc
Ah oops, missed that somehow.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@259
PS4, Line 259: wal
nit: WAL. Same elsewhere
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@266
PS4, Line 266: "Total space available in all the data directories",
We should also mention the error behavior that this returns -1
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@431
PS4, Line 431: // Calculates the free space on the WAL disk.
nit: could you also document the error semantics? Same with the other method.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@432
PS4, Line 432: int64_t
nit: this doesn't need to have a return value if values are being returned via output parameters.
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434
PS4, Line 434: waldir_space_last_check
nit: we should keep the naming of our variables standardized as separating words with underscores. I.e. change this to "wal_dir_space_last_check_". Same elsewhere, and with "datadir" --> "data_dir"
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@434
PS4, Line 434: MonoTime& waldir_space_last_check,
: int64_t& wal_dir_available_space
nit: parameters that are output values (even if they're also input values) tend to be pointers, not references. I.e. this should be MonoTime* and int64_t*
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@443
PS4, Line 443: wal_dir_available_space = -1;
: return wal_dir_available_space;
As a user, I'm curious if it would make sense to leave the value as is on failure. I.e. if there's a failed disk, would it make sense to just not update the value, rather than using a magic number?
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@447
PS4, Line 447: reserved_bytes = (should_reserve_one_percent == TRUE) ?
> warning: result of comparison of constant 'RLE' (4) with expression of type
should_preserve_one_percent is already a boolean. You don't need further comparison against true/false
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Aug 2021 17:37:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: // Calculates the total free space on all the data directories combined accounting for
> So if I am understanding it right, you are suggesting a new function which
Yes. It could be inlined too if you want to avoid the overhead of calling a function.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 30 Jul 2021 13:12:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG@26
PS7, Line 26: the
> nit: remove
Done
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG@27
PS7, Line 27: fs_data_dirs_available_space_cache_seconds
> nit: add --
Done
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc@85
PS7, Line 85: . "
> nit: there's an extra space here too
Done
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc@85
PS7, Line 85:
> nit: in the
Done
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/integration-tests/disk_failure-itest.cc@357
PS7, Line 357:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/server/server_base.cc@269
PS7, Line 269: l"
> nit: fails
Done
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/util/env.h@49
PS7, Line 49:
> nit: remove the extra newline
Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 17 Aug 2021 21:57:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#14).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 186 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/14
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 16: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h
File src/kudu/server/server_base.h:
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@269
PS16, Line 269: mutable
nit: I guess 'mutable' isn't needed here since the methods which acquire the lock aren't marked 'const'
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@271
PS16, Line 271: it's important that this is the first member to be destructed
style: to keep this comment easier to follow in the future modifications, please move the declaration of the two RefreshXxx() methods below to line 254?
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@275
PS16, Line 275: DISALLOW_COPY_AND_ASSIGN(ServerBase);
nit: for better readability, prefer to keep this in the very end of the class
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.h
File src/kudu/server/server_base.h:
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.h@195
PS14, Line 195:
: int64_t wal_dir_available_space_;
: int64_t data_dirs_available_space_;
: MonoTime wal_dir_space_last_check_;
Why not to move this into the private section to be along with the 'lock_' member field? If these are guarded by 'lock_', then it would be strange to expose those to the descendant classes allowing access without acquiring the lock?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 27 Aug 2021 15:51:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#4).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
The free space calculated is cached for the period specified by the the
flag fs_data_dirs_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
6 files changed, 103 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/4
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 14:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@919
PS14, Line 919: wal_dir_available_space_
I was going to point out that we need to take the lock before returning this. Instead, I'd suggest having RefreshWalDir... return the space to avoid the extra taking of the lock, now that we don't have any output parameters.
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@953
PS14, Line 953: MonoTime expiry = wal_dir_space_last_check_ + MonoDelta::FromSeconds(
: FLAGS_fs_wal_dir_available_space_cache_seconds);
We also need to hold the lock here. Consider early returning when possible to eliminate the extra indentation of added scopes. E.g.
void ServerBase::Refresh... {
{
std::lock_guard<simple_spinlock> l(lock_);
if (MonoTime::Now() < wal_dir_space_last_check_ + MonoDelta::FromSeconds(FLAGS...) {
return wal_dir_available_space_;
}
}
SpaceInfo space_info_waldir;
int64_t wal_dir_space = ...
...
std::lock_guard<simple_spinlock> l(lock_);
wal_dir_space_last_check_ = MonoTime::Now();
return wal_dir_available_space_;
}
Same for data dirs
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@958
PS14, Line 958:
nit: small spacing alignment
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@974
PS14, Line 974: &
nit: const string& data_dir
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:24:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(2 comments)
Not much more to add on top of what Alexey mentioned.
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt
File src/kudu/server/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt@62
PS2, Line 62: consensus
> Yes, it is needed to access the flag fs_wal_dir_reserved_bytes which is def
Could we move the definition of the fs_wal_dir_reserved_bytes to, e.g. fs_manager.cc, and only DECLARE_int64() in consensus/log.cc? It does seem a bit odd to include 'consensus' here, given there isn't a strong connection between it and the sever module.
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@431
PS2, Line 431: ServerBaseOptions options
> Why not to pass ServerBaseOptions as a const reference?
+1
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 27 Jul 2021 20:35:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#16).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 191 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/16
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 15: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/15/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/15/src/kudu/server/server_base.cc@972
PS15, Line 972: wal
data_dir
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 15
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 27 Aug 2021 00:35:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17725/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/2//COMMIT_MSG@10
PS2, Line 10: wal_dir_space_available_bytes - Exposes the free space available in the
nit: can you make these bullet points for better readability? ("-" or "*")
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: total_free_space +=
> There might be dragons here: what if (space_info_datadir.free_bytes - FLAGS
One more dragon I can think of: if (space_info_datadir.free_bytes - FLAGS_fs_data_dirs_reserved_bytes) is negative for any of the dirs, total_free_space will be less than the real total free space.
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: total_free_space += space_info_datadir.free_bytes - FLAGS_fs_data_dirs_reserved_bytes;
maybe this could be refactored so that both GetDataDirAvailableSpace and GetWalDirAvailable space use the same function to get the available space in a single directory.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 28 Jul 2021 11:44:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt
File src/kudu/server/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt@62
PS2, Line 62: consensus
> +1 to the approach suggested by Andrew.
Done
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/server_base.cc@446
PS2, Line 446: total_free_space += space_info_datadir.free_bytes - FLAGS_fs_data_dirs_reserved_bytes;
> maybe this could be refactored so that both GetDataDirAvailableSpace and Ge
So if I am understanding it right, you are suggesting a new function which returns the free space in a given directory and this can be used by GetDataDirAvailableSpace and GetWalDirAvailable?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 29 Jul 2021 21:47:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: y) {
> Yes, that makes sense. Done.
This doesn't return anything when we're passed our expiration. Having this as an out parameter lets us no-op if the metric hasn't expired.
http://gerrit.cloudera.org:8080/#/c/17725/11/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/11/src/kudu/server/server_base.cc@459
PS11, Line 459: return(CalculateAvailableSpace(options, fs_manager.GetWalsRootDir(),
:
nit: "return CalculateAvailable...("
Same elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Aug 2021 17:28:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#2).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/server/CMakeLists.txt
M src/kudu/server/server_base.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
4 files changed, 52 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/2
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/4/src/kudu/server/server_base.cc@437
PS4, Line 437: FLAGS_fs_data_dirs_available_space_cache_seconds
Not sure if we want to use a separate flag for wal dir caching?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Aug 2021 15:14:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#5).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
The free space calculated is cached for the period specified by the the
flag fs_data_dirs_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 164 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/5
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: y) {
> This doesn't return anything when we're passed our expiration. Having this
Ah, right thanks for pointing that out. Knew there was a reason for that but couldn't recollect it.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Aug 2021 17:50:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#17).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 192 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/17
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 17
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 16:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h
File src/kudu/server/server_base.h:
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@269
PS16, Line 269: mutable
> nit: I guess 'mutable' isn't needed here since the methods which acquire th
Done
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@271
PS16, Line 271: it's important that this is the first member to be destructed
> style: to keep this comment easier to follow in the future modifications, p
Done
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@275
PS16, Line 275: DISALLOW_COPY_AND_ASSIGN(ServerBase);
> nit: for better readability, prefer to keep this in the very end of the cla
Done
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@290
PS16, Line 290:
> nit: extra spaces
Done
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.h
File src/kudu/server/server_base.h:
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.h@195
PS14, Line 195:
: int64_t wal_dir_available_space_;
: int64_t data_dirs_available_space_;
: MonoTime wal_dir_space_last_check_;
> Why not to move this into the private section to be along with the 'lock_'
Done
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@919
PS14, Line 919: achToLastValue(&metric_d
> I was going to point out that we need to take the lock before returning thi
Done
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@953
PS14, Line 953: if (MonoTime::Now() < wal_dir_space_last_check_ + MonoDelta::FromSeconds(
: FLAGS_fs_wal_dir_available_space_cache_seconds
> We also need to hold the lock here. Consider early returning when possible
Done
http://gerrit.cloudera.org:8080/#/c/17725/14/src/kudu/server/server_base.cc@958
PS14, Line 958: (
> nit: small spacing alignment
Done
http://gerrit.cloudera.org:8080/#/c/17725/15/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/15/src/kudu/server/server_base.cc@972
PS15, Line 972: dat
> data_dir
Done
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Aug 2021 01:13:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 9: Code-Review+1
(3 comments)
Missed the test in my last iteration.
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@325
PS9, Line 325: We could do ASSERT_EQ of the metrics and the current available
: // space but there will most likely be some data written between the two calls causing
: // a failure.
Sure we can't check that they're equal to each other, but could we check that the second time we get them they're the same as the previous time? I think that should be the case, given we cache the values. Consider fetching the metric again before restarting to exercise the caching functionality.
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@330
PS9, Line 330: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value",
: &wal_dir_space));
: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value",
: &data_dir_space));
:
nit: Given we're using this in a few places now (and maybe one more to come?), consider wrapping it in a lambda, defined just once. I.e.
const auto get_metrics = [&] (int64_t* wal_dir_space, int64_t* data_dir_space) {
RETURN_NOT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
&METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value",
wal_dir_space));
return itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
&METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value",
data_dir_space);
};
int64_t wal_dir_space;
int64_t data_dir_space;
ASSERT_OK(get_metrics(&wal_dir_space, &data_dir_space));
http://gerrit.cloudera.org:8080/#/c/17725/9/src/kudu/integration-tests/disk_failure-itest.cc@346
PS9, Line 346: Substitute(
nit: there are no substitutions happening, so we can remove this.
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 18 Aug 2021 17:43:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 10: Verified+1
(16 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@22
PS10, Line 22: mac
nit: macOS ?
http://gerrit.cloudera.org:8080/#/c/17725/10//COMMIT_MSG@24
PS10, Line 24: directores
nit: directories
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/fs/data_dirs.cc@84
PS10, Line 84: fs_wal_dir_available_space_cache_seconds
Is it intentional that setting this flag to a negative value leads to always stale value of the available WAL dir space metric? If not, maybe add a flag validator for this and the --fs_data_dirs_available_space_cache_seconds flag (something like we have for the --fs_wal_dir_reserved_bytes flag).
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@260
PS10, Line 260: WAL directory space free"
nit: usually, the metric's label hash all words started with capital letter, something like "WAL Directory Free Space"
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@262
PS10, Line 262: Returns "
: "-1 if reading the disk fails
nit: maybe rephrase as
Set to -1 if failed to retrieve disk metadata
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@266
PS10, Line 266: "data directories space free"
nit: "Data Directories Free Space"
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@268
PS10, Line 268: Returns "
: "-1 if reading any of the disks fails
nit:
Set to -1 if failed to retrieve metadata of any of the disks
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@438
PS10, Line 438: int64_t reserved_bytes;
: bool should_reserve_one_percent;
nit: move these down where they assigned with their values, turning these declarations into definitions?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@446
PS10, Line 446: static_cast<int64_t> (0)
nit: could replace with 0LL ?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@456
PS10, Line 456: int64_t* wal_dir_available_space
Why not to return this from this function? Why to keep this as an output parameter?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@474
PS10, Line 474:
indentation nit: remove one space here and for two lines below
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@476
PS10, Line 476: int64_t* data_dirs_available_space
Why to have this as an output parameter? Maybe, just return the calculated value from this function?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@479
PS10, Line 479: SpaceInfo space_info_datadir;
nit: move this under the 'if()' scope below?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@483
PS10, Line 483: int64_t available_space;
nit: move this under the scope of the 'for()' cycle below?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/server/server_base.cc@494
PS10, Line 494: data_dirs_available_space
What if data_dirs_available_space points to non-zeroed memory? Is it intentional just to add up to the value passed into this function?
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/util/env.h@48
PS10, Line 48: q:q
nit: drop these trailing symbols?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Aug 2021 02:04:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Reviewed-on: http://gerrit.cloudera.org:8080/17725
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 191 insertions(+), 10 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 18
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 7: Code-Review+1
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG@26
PS7, Line 26: the
nit: remove
http://gerrit.cloudera.org:8080/#/c/17725/7//COMMIT_MSG@27
PS7, Line 27: fs_data_dirs_available_space_cache_seconds
nit: add --
maybe also mention the new flag for WALs?
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc@85
PS7, Line 85:
nit: in the
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/fs/data_dirs.cc@85
PS7, Line 85: . "
nit: there's an extra space here too
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/integration-tests/disk_failure-itest.cc@357
PS7, Line 357:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/server/server_base.cc@269
PS7, Line 269: l"
nit: fails
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/util/env.h
File src/kudu/util/env.h:
http://gerrit.cloudera.org:8080/#/c/17725/7/src/kudu/util/env.h@49
PS7, Line 49:
nit: remove the extra newline
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 17 Aug 2021 20:49:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 2:
(1 comment)
Will take care of the rest of the pointers in the next patch
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt
File src/kudu/server/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/17725/2/src/kudu/server/CMakeLists.txt@62
PS2, Line 62: consensus
> I'm not sure I understand why this is now required here: server_process is
Yes, it is needed to access the flag fs_wal_dir_reserved_bytes which is defined in consensus/log.cc. Wasn't sure getting the flag value through any other means would be more efficient. Any other suggestions to get the flag value?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 27 Jul 2021 16:10:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc@953
PS13, Line 953: MonoTime expiry = wal_dir_space_last_check_ + MonoDelta::FromSeconds(
: FLAGS_fs_wal_dir_available_space_cache_seconds);
: if (MonoTime::Now() > expiry) {
: wal_dir_space_last_check_ = MonoTime::Now();
: SpaceInfo space_info_waldir;
: wal_dir_available_space_ = CalculateAvailableSpace(options, fs_manager.GetWalsRootDir(),
: FLAGS_fs_wal_dir_reserved_bytes,
: &space_info_waldir);
: }
: }
> I have a concern about thread safety that was actually true with the previo
Done
http://gerrit.cloudera.org:8080/#/c/17725/13/src/kudu/server/server_base.cc@972
PS13, Line 972: data_dirs_available_space_ = 0;
> We shouldn't be setting this to 0 every time we call this function, right?
Right, fixed it now in the next patch
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 13
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:11:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#10).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags fs_data_dirs_reserved_bytes and fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local mac machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directores on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 184 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/10
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 10: -Verified Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Aug 2021 02:05:21 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 10: Verified+1 Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc
File src/kudu/integration-tests/disk_failure-itest.cc:
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@352
PS10, Line 352: E
nit: got a couple extra spaces here
http://gerrit.cloudera.org:8080/#/c/17725/10/src/kudu/integration-tests/disk_failure-itest.cc@364
PS10, Line 364: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_wal_dir_space_available_bytes, "value",
: &wal_dir_space));
: ASSERT_OK(itest::GetInt64Metric(cluster_->tablet_server(0)->bound_http_hostport(),
: &METRIC_ENTITY_server, nullptr, &METRIC_data_dirs_space_available_bytes, "value",
: &data_dir_space));
: ASSERT_NE(wal_dir_space, -1);
nit: we can use get_metrics() here too, right?
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Aug 2021 00:47:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17725
to look at the new patch set (#13).
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
KUDU-3204: Add a metric for amount of available space
This commit adds two metrics to server entity.
- wal_dir_space_available_bytes - Exposes the free space available in the
wal directory
- data_dirs_space_available_bytes - Exposes the sum of free space in each
of the data directories.
We ensure the reserved space is being accounted for. This is specified by
the flags --fs_data_dirs_reserved_bytes and --fs_wal_dir_reserved_bytes in
data directories and wal directories respectively. For calculating the
total free space in data directories, we ensure we are not counting the
same device/filesystem twice by getting and checking for uniqueness
of the filesystemID of each of the data directories before summing it up.
Tested the uniqueness of the filesystemID by creating a new volume in my
local macOS machine. The space reported is based on the data directories in
different volumes (and hence unique filesystemID). Having multiple data
directories on the same volume does not increase the metric.
The free space calculated is cached for the period specified by the flags
--fs_data_dirs_available_space_cache_seconds and
--fs_wal_dir_available_space_cache_seconds.
Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
---
M src/kudu/consensus/log.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
8 files changed, 178 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/17725/13
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 13
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17725 )
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Patch Set 16: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h
File src/kudu/server/server_base.h:
http://gerrit.cloudera.org:8080/#/c/17725/16/src/kudu/server/server_base.h@290
PS16, Line 290:
nit: extra spaces
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 28 Aug 2021 07:03:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3204: Add a metric for amount of available space
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3204: Add a metric for amount of available space
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17725
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I58a7419847d080498aaf431d1dab098e1af71ad0
Gerrit-Change-Number: 17725
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)