You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2019/04/06 01:37:41 UTC
[Impala-ASF-CR] IMPALA-7981: Add host disk usage to profile
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12822 )
Change subject: IMPALA-7981: Add host disk usage to profile
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info-test.cc
File be/src/util/system-state-info-test.cc:
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info-test.cc@105
PS2, Line 105: R"( 8 0 sda 17124835 222797 716029974 8414920 43758807 38432095 7867287712 630179264 0 32547524 638999340
> line too long (117 > 90)
Do we also care about testing for random devices which show up in /proc/diskstats such as ram0 ? My /proc/diskstats has the followings:
1 0 ram0 0 0 0 0 0 0 0 0 0 0 0
1 1 ram1 0 0 0 0 0 0 0 0 0 0 0
1 2 ram2 0 0 0 0 0 0 0 0 0 0 0
1 3 ram3 0 0 0 0 0 0 0 0 0 0 0
1 4 ram4 0 0 0 0 0 0 0 0 0 0 0
1 5 ram5 0 0 0 0 0 0 0 0 0 0 0
1 6 ram6 0 0 0 0 0 0 0 0 0 0 0
1 7 ram7 0 0 0 0 0 0 0 0 0 0 0
1 8 ram8 0 0 0 0 0 0 0 0 0 0 0
1 9 ram9 0 0 0 0 0 0 0 0 0 0 0
1 10 ram10 0 0 0 0 0 0 0 0 0 0 0
1 11 ram11 0 0 0 0 0 0 0 0 0 0 0
1 12 ram12 0 0 0 0 0 0 0 0 0 0 0
1 13 ram13 0 0 0 0 0 0 0 0 0 0 0
1 14 ram14 0 0 0 0 0 0 0 0 0 0 0
1 15 ram15 0 0 0 0 0 0 0 0 0 0 0
7 0 loop0 0 0 0 0 0 0 0 0 0 0 0
7 1 loop1 0 0 0 0 0 0 0 0 0 0 0
7 2 loop2 0 0 0 0 0 0 0 0 0 0 0
7 3 loop3 0 0 0 0 0 0 0 0 0 0 0
7 4 loop4 0 0 0 0 0 0 0 0 0 0 0
7 5 loop5 0 0 0 0 0 0 0 0 0 0 0
7 6 loop6 0 0 0 0 0 0 0 0 0 0 0
7 7 loop7 0 0 0 0 0 0 0 0 0 0 0
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info-test.cc@177
PS2, Line 177: // Tests the computation logic for disk usage. This test also makes sure that values of a
: // partition are not accounted towards the disk it is on.
Is this comment meant for the earlier test at line 103 ?
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.h
File be/src/util/system-state-info.h:
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.h@183
PS2, Line 183: /// The enum names correspond to the fields of /proc/diskstats
: /// https://www.kernel.org/doc/Documentation/iostats.txt
The followings are the metrics common to both 2.6 and 4.8+, right ?
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.h@221
PS2, Line 221: Entries are
: /// set to 0 for invalid entries.
Invalid entries are set to 0.
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.h@228
PS2, Line 228: /// Compute the disk usage.
Computes the read and write rate for the most recent epoch.
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.h@233
PS2, Line 233: DiskUsage
nit: I kind of interpret it as disk space used on first read. DiskStats may be a slightly better name but then it may not be consistent with other metrics we are measuring.
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.cc
File be/src/util/system-state-info.cc:
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.cc@45
PS2, Line 45: memset(&network_usage_, 0, sizeof(network_usage_));
memset(&disk_usage_, 0, sizeof(disk_usage_));
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.cc@81
PS2, Line 81: LOG(WARNING) << "Could not open " << path << ": " << GetStrErrMsg() << endl;
Will this lead to spam in the log if we periodically try reading the same path and fail or is it the responsibility of the caller to throttle calling this function ?
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.cc@199
PS2, Line 199:
DCHECK(disk_val_idx_ == 0 || disk_val_idx_ == 1);
http://gerrit.cloudera.org:8080/#/c/12822/2/be/src/util/system-state-info.cc@232
PS2, Line 232: ==
<= 0
--
To view, visit http://gerrit.cloudera.org:8080/12822
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57
Gerrit-Change-Number: 12822
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Apr 2019 01:37:41 +0000
Gerrit-HasComments: Yes