You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org> on 2019/10/16 01:03:12 UTC
[kudu-CR] [ksck] KUDU-2964 Summarize output when GetFlags returns error
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14460
Change subject: [ksck] KUDU-2964 Summarize output when GetFlags returns error
......................................................................
[ksck] KUDU-2964 Summarize output when GetFlags returns error
When GetFlags returns error then summarize the failure
grouped by the return code in the ksck output.
Tests:
- Invoked ksck against KUDU version 1.4.0.
unable to get flag information from 2 tablet server(s)
[4391199870774acd838b6cce1bb49da4 (bankim-512-3.gce.cloudera.com:7050),
d35ce3b9bbc343e0ae65a1163dc48bc2 (bankim-512-2.gce.cloudera.com:7050), ]
max 5 because of "Remote error". For tablet server
4391199870774acd838b6cce1bb49da4: Invalid argument: Call on service
kudu.server.GenericService received at 172.31.112.181:7050 from
10.16.0.67:60224 with an invalid method name: GetFlags
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
2 files changed, 46 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/1
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 1
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc@1240
PS4, Line 1240: " Flag | Value | Tags | Tablet Server\n"
: "--------------------+-------+---------------+-------------------------------\n"
: " experimental_flag | x | experimental | all 3 server(s) checked\n"
:
> This way the tests related to "flags" are next to each other for master and
Sure, I don't feel strongly about it.
http://gerrit.cloudera.org:8080/#/c/14460/5/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/5/src/kudu/tools/ksck-test.cc@920
PS5, Line 920: void CheckMessageNotPresent(const vector<Status>& messages, const string& msg) {
: ASSERT_FALSE(std::any_of(messages.begin(), messages.end(),
: [&msg](const Status& status) {
: return status.ToString().find(msg) != string::npos;
: }));
: }
nit: You can use ASSERT_STR_NOT_CONTAINS() for this
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 18:30:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 03:07:18 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 21:05:58 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14460
to look at the new patch set (#5).
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
[ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Logging individual master/tablet server warning message on
failure to fetch flags is unnecessarily verbose since
the cause is almost always the same, either the server is unavailable
or GetFlags RPC is not available because server is old.
There is already a summary of number of masters/tablet servers
that failed to respond to GetFlags request. So remove the per server
warning message.
Tests:
- Invoked ksck against KUDU version 1.4.0.
Warnings:
==================
master flag check error: 1 of 1 masters' flags were not available
tserver flag check error: 2 of 2 tservers' flags were not available
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
2 files changed, 73 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/5
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
[ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Logging individual master/tablet server warning message on
failure to fetch flags is unnecessarily verbose since
the cause is almost always the same, either the server is unavailable
or GetFlags RPC is not available because server is old.
There is already a summary of number of masters/tablet servers
that failed to respond to GetFlags request. So remove the per server
warning message.
Tests:
- Invoked ksck against KUDU version 1.4.0.
Warnings:
==================
master flag check error: 1 of 1 masters' flags were not available
tserver flag check error: 2 of 2 tservers' flags were not available
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Reviewed-on: http://gerrit.cloudera.org:8080/14460
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
2 files changed, 72 insertions(+), 32 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 7
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck-test.cc@64
PS3, Line 64: DEFINE_bool(test_get_flags_available, true, "Whether GetFlags RPC is available");
> Doesn't seem like you need to plumb this in as a gflag, especially since us
Thanks for the details, Adar.
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck.cc@209
PS3, Line 209: // Failure to gather flags is not supported in older versions and is tracked in
: // CheckMasterUnusualFlags().
> How about "Flag retrieval is not supported by older versions; failure is tr
Done
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 00:58:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14460
to look at the new patch set (#6).
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
[ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Logging individual master/tablet server warning message on
failure to fetch flags is unnecessarily verbose since
the cause is almost always the same, either the server is unavailable
or GetFlags RPC is not available because server is old.
There is already a summary of number of masters/tablet servers
that failed to respond to GetFlags request. So remove the per server
warning message.
Tests:
- Invoked ksck against KUDU version 1.4.0.
Warnings:
==================
master flag check error: 1 of 1 masters' flags were not available
tserver flag check error: 2 of 2 tservers' flags were not available
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
2 files changed, 72 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/6
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 6
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc@1111
PS4, Line 1111: ASSERT_TRUE(ksck_->CheckMasterUnusualFlags().IsIncomplete());
> Maybe check the output of running the ksck and checking that there's no "invalid method name: GetFlags" messages?
>
Done.
> Also, what was the error status before this patch? Would this test fail without this patch?
Now with the changes incorporated in the test case (patchset 5), the test would fail without changes incorporated in ksck.cc.
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc@1240
PS4, Line 1240: TEST_F(GetFlagsUnavailableKsckTest, TestTserverFlagsUnavailable) {
: ASSERT_OK(ksck_->FetchInfoFromTabletServers());
: ASSERT_TRUE(ksck_->CheckTabletServerUnusualFlags().IsIncomplete());
: }
> nit: could you move this up so all the GetFlagsUnavailableKsckTest cases ar
This way the tests related to "flags" are next to each other for master and tablet servers, hence added them in this order. I'd prefer this way unless you strongly prefer test fixtures to be together.
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 18:10:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Summarize output when GetFlags returns error
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Summarize output when GetFlags returns error
......................................................................
Patch Set 2:
(2 comments)
I appreciate the detailed solution, but I think we can get by with something simpler: what if we just didn't add the failure to results_.warning_messages?
AFAICT we already get a "summary" of sorts in CheckMasterUnusualFlags/CheckTabletServerUnusualFlags (look at the use of the 'bad_masters' local variable). I expect GetFlags failures to occur either if the remote process is down (in which case the rest of ksck's access to that process will also fail), or if it's too old. So there's not much value in a fine grained "group by" on the GetFlags failures.
http://gerrit.cloudera.org:8080/#/c/14460/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14460/1//COMMIT_MSG@11
PS1, Line 11:
: Tests:
: - Invoked ksck against KUDU version 1.4.0.
Would be better if we had an automated test. Perhaps you could add a test-only gflag that causes GetFlags to fail?
http://gerrit.cloudera.org:8080/#/c/14460/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/14460/1/src/kudu/tools/ksck.cc@210
PS1, Line 210: std::lock_guard<simple_spinlock> lock(master_summaries_lock);
The solution for tserver flags should also be adopted for master flags.
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 16 Oct 2019 03:10:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Summarize output when GetFlags returns error
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14460
to look at the new patch set (#2).
Change subject: [ksck] KUDU-2964 Summarize output when GetFlags returns error
......................................................................
[ksck] KUDU-2964 Summarize output when GetFlags returns error
When GetFlags returns error then summarize the failure
grouped by the return code in the ksck output.
Tests:
- Invoked ksck against KUDU version 1.4.0.
unable to get flag information from 2 tablet server(s)
[4391199870774acd838b6cce1bb49da4 (bankim-512-3.gce.cloudera.com:7050),
d35ce3b9bbc343e0ae65a1163dc48bc2 (bankim-512-2.gce.cloudera.com:7050), ]
max 5 because of "Remote error". For tablet server
4391199870774acd838b6cce1bb49da4: Invalid argument: Call on service
kudu.server.GenericService received at 172.31.112.181:7050 from
10.16.0.67:60224 with an invalid method name: GetFlags
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
2 files changed, 48 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/2
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 18:34:10 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14460
to look at the new patch set (#4).
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
[ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Logging individual master/tablet server warning message on
failure to fetch flags is unnecessarily verbose since
the cause is almost always the same, either the server is unavailable
or GetFlags RPC is not available because server is old.
There is already a summary of number of masters/tablet servers
that failed to respond to GetFlags request. So remove the per server
warning message.
Tests:
- Invoked ksck against KUDU version 1.4.0.
Warnings:
==================
master flag check error: 1 of 1 masters' flags were not available
tserver flag check error: 2 of 2 tservers' flags were not available
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
2 files changed, 60 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/4
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14460/5/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/5/src/kudu/tools/ksck-test.cc@920
PS5, Line 920: void CheckMessageNotPresent(const vector<Status>& messages, const string& msg) {
: ASSERT_FALSE(std::any_of(messages.begin(), messages.end(),
: [&msg](const Status& status) {
: return status.ToString().find(msg) != string::npos;
: }));
: }
> nit: You can use ASSERT_STR_NOT_CONTAINS() for this
Done
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 5
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 20:08:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14460
to look at the new patch set (#3).
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
[ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Logging individual master/tablet server warning message on
failure to fetch flags is unnecessarily verbose since
the cause is almost always the same, either the server is unavailable
or GetFlags RPC is not available because server is old.
There is already a summary of number of masters/tablet servers
that failed to respond to GetFlags request. So remove the per server
warning message.
Tests:
- Invoked ksck against KUDU version 1.4.0.
Warnings:
==================
master flag check error: 1 of 1 masters' flags were not available
tserver flag check error: 2 of 2 tservers' flags were not available
Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
2 files changed, 36 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/14460/3
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [ksck] KUDU-2964 Summarize output when GetFlags returns error
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Summarize output when GetFlags returns error
......................................................................
Patch Set 2:
(2 comments)
> Patch Set 2:
>
> (2 comments)
>
> I appreciate the detailed solution, but I think we can get by with something simpler: what if we just didn't add the failure to results_.warning_messages?
>
> AFAICT we already get a "summary" of sorts in CheckMasterUnusualFlags/CheckTabletServerUnusualFlags (look at the use of the 'bad_masters' local variable). I expect GetFlags failures to occur either if the remote process is down (in which case the rest of ksck's access to that process will also fail), or if it's too old. So there's not much value in a fine grained "group by" on the GetFlags failures.
Makes sense.
http://gerrit.cloudera.org:8080/#/c/14460/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14460/1//COMMIT_MSG@11
PS1, Line 11:
: Tests:
: - Invoked ksck against KUDU version 1.4.0.
> Would be better if we had an automated test. Perhaps you could add a test-o
Done
http://gerrit.cloudera.org:8080/#/c/14460/1/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/14460/1/src/kudu/tools/ksck.cc@210
PS1, Line 210: std::lock_guard<simple_spinlock> lock(master_summaries_lock);
> The solution for tserver flags should also be adopted for master flags.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 2
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 17 Oct 2019 22:48:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck-test.cc@64
PS3, Line 64: DEFINE_bool(test_get_flags_available, true, "Whether GetFlags RPC is available");
Doesn't seem like you need to plumb this in as a gflag, especially since users of the test aren't expected to change its value when running the test. Instead:
1. Make it a constructor arg to MockKsckMaster/MockKsckTabletServer, stored in new state in each.
2. Subclass KsckTest and add a virtual function that statically determines whether to disable GetFlags (it'd be enabled in KsckTest and disabled in the subclass' implementation).
3. Modify KsckTest::Setup to call the virtual function and use it to determine what value to pass to the constructor of MockKsckMaster/MockKsckTabletServer.
4. Use the subclass test fixture for your new tests.
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/14460/3/src/kudu/tools/ksck.cc@209
PS3, Line 209: // Failure to gather flags is not supported in older versions and is tracked in
: // CheckMasterUnusualFlags().
How about "Flag retrieval is not supported by older versions; failure is tracked in CheckMasterUnusualFlags."
Same below.
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 17 Oct 2019 22:58:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14460 )
Change subject: [ksck] KUDU-2964 Remove per server warn message when GetFlags call fails
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc@1111
PS4, Line 1111: ASSERT_TRUE(ksck_->CheckMasterUnusualFlags().IsIncomplete());
Maybe check the output of running the ksck and checking that there's no "invalid method name: GetFlags" messages?
Also, what was the error status before this patch? Would this test fail without this patch?
http://gerrit.cloudera.org:8080/#/c/14460/4/src/kudu/tools/ksck-test.cc@1240
PS4, Line 1240: TEST_F(GetFlagsUnavailableKsckTest, TestTserverFlagsUnavailable) {
: ASSERT_OK(ksck_->FetchInfoFromTabletServers());
: ASSERT_TRUE(ksck_->CheckTabletServerUnusualFlags().IsIncomplete());
: }
nit: could you move this up so all the GetFlagsUnavailableKsckTest cases are near each other?
--
To view, visit http://gerrit.cloudera.org:8080/14460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06dd417b9bf5274f90bbdb9338600ea518e82809
Gerrit-Change-Number: 14460
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2019 01:58:57 +0000
Gerrit-HasComments: Yes