You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/02/26 08:19:18 UTC
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15298
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
WIP [ksck] report on misconfiguration of time source flags
This patch adds functionality to report on misconfiguration of the time
source specific flags in Kudu cluster. Particularly, with this patch
ksck issues a warning if different masters or different servers have
different settings for the following flags:
* --time_source
* --builtin_ntp_servers
For testing, I ran a small Kudu cluster and verified that ksck
reports on misconfiguration as expected.
WIP:
* collect feedback on this approach: maybe, a better option is to
introduce new RPC methods into the GenericService interface
(see src/kudu/server/server_base.proto for details)?
* add automated tests to exercise the new functionality
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
---
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
7 files changed, 252 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/15298/1
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@10
PS1, Line 10: Particularly, with this patch
: ksck issues a warning if different masters or different servers have
: different settings for the following flags:
: * --time_source
: * --builtin_ntp_servers
> When ksck prints a warning, does it also exit with a non-zero status? If so
Printing warnings doesn't yield non-zero exit status. Additionally, in PS2, the verification of the uniformity for flags settings runs only if a particular flag is present.
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@20
PS1, Line 20: * collect feedback on this approach: maybe, a better option is to
: introduce new RPC methods into the GenericService interface
: (see src/kudu/server/server_base.proto for details)?
> FWIW, I think this flag-based approach is a good one.
Thank you for the feedback. OK, let's keep it then :)
http://gerrit.cloudera.org:8080/#/c/15298/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:
http://gerrit.cloudera.org:8080/#/c/15298/1/src/kudu/tools/ksck.h@598
PS1, Line 598: // Check for the consistent settings of the time source-related flags among
: // all tablet servers in the cluster.
: // Must first call FetchInfoFromTabletServers().
: Status CheckTabletServerTimeSourceFlags();
> I like this, but can you restructure the code so that it's not specific to
Done.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Mar 2020 05:38:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@10
PS1, Line 10: Particularly, with this patch
: ksck issues a warning if different masters or different servers have
: different settings for the following flags:
: * --time_source
: * --builtin_ntp_servers
Why is it important that all masters or all tservers share the same values for these flags? Moreover, if it's important, shouldn't we enforce that _all servers_ share those values, rather than all masters in one check and all tservers in another?
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@20
PS1, Line 20: * collect feedback on this approach: maybe, a better option is to
: introduce new RPC methods into the GenericService interface
: (see src/kudu/server/server_base.proto for details)?
FWIW, I think this flag-based approach is a good one.
http://gerrit.cloudera.org:8080/#/c/15298/1/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:
http://gerrit.cloudera.org:8080/#/c/15298/1/src/kudu/tools/ksck.h@598
PS1, Line 598: // Check for the consistent settings of the time source-related flags among
: // all tablet servers in the cluster.
: // Must first call FetchInfoFromTabletServers().
: Status CheckTabletServerTimeSourceFlags();
I like this, but can you restructure the code so that it's not specific to time-related flags? I can see us gradually adding more and more flags to this list, so that we can enforce that those flags have equal values across the entirety of the cluster. We can certainly start with these time flags, but ultimately the approach is generic.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:26:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2020 16:07:23 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@12
PS2, Line 12: differences between masters' and tablet servers' settings
: for same flags.
Isn't this redundant w.r.t. the first half of the sentence?
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@21
PS2, Line 21: To use the newly introduced functionality, add
: --flags_categories_to_check=<comma-separated-list-of-flags-categories>
: to the 'kudu cluster ksck' invocation.
Thoughts on enabling this by default?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@57
PS2, Line 57: #define STR_FLAGS_CATEGORY_TIME_SOURCE "time_source"
: #define STR_FLAGS_CATEGORY_UNUSUAL "unusual"
Could use C-string constants?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@245
PS2, Line 245: SplitStringUsing(str, ",", &categories_str);
I think we typically using strings::Split rather than this. You can embed it directly in the loop.
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@390
PS2, Line 390: void Ksck::AddFlagsToFlagMap(const GetFlagsResponsePB& flags,
: const string& server_address,
: KsckFlagToServersMap* flags_to_servers_map) {
: CHECK(flags_to_servers_map);
: for (const auto& f : flags.flags()) {
: const pair<string, string> key(f.name(), f.value());
: if (!InsertIfNotPresent(flags_to_servers_map, key, { server_address })) {
: FindOrDieNoPrint(*flags_to_servers_map, key).push_back(server_address);
: }
: }
: }
Seems pretty straight-forward to reuse the existing AddFlagsToFlagMaps with a null flag_tags_map?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@722
PS2, Line 722: set<KsckFlag> masters_flags;
Could use unordered_sets here?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@753
PS2, Line 753: // The flag/value pair must be either of masters' or tservers'.
: DCHECK(false);
LOG(DFATAL) instead?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h
File src/kudu/tools/ksck_results.h:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h@174
PS2, Line 174: // Print a formatted summary of the flags in 'flag_to_servers_map',
: // indicating which servers have which (flag, value) pairs set.
Could you reword this in terms of the existing PrintFlagTable, or vice versa?
Would also be nice to avoid the overload; could you rename one?
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Mar 2020 07:06:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
[ksck] report on misconfiguration for flag categories
This patch adds functionality to report on misconfiguration of flags
in pre-defined flag categories. The 'misconfiguration' means different
values for same flags in each group of masters/tablet servers in a
cluster (i.e. intra-group differences), and also differences between
masters' and tablet servers' flags (i.e. cross-group differences).
With this patch, two categories are defined and ready for use:
* 'time_source' includes the following flags
** --builtin_ntp_servers
** --time_source
* 'unusual' includes all experimental, hidden, and unsafe flags
The newly introduced functionality is enabled by default for the
'time_source' flags category. The list of group categories to check
can be customized by adding
--flags_categories_to_check=<comma-separated-list-of-flags-categories>
to the 'kudu cluster ksck' invocation. Use an empty string to specify
an empty list of flags categories to check.
For testing, I ran a small Kudu cluster and verified that ksck reports
on misconfiguration as expected:
* if tablet servers are run with different flags
* if masters are run with flag settings different from those used for
tablet servers
This patch also contains assorted re-factoring in related areas
of code. I'm planning to add automated tests to cover the newly
introduced functionality in a separate patch.
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Reviewed-on: http://gerrit.cloudera.org:8080/15298
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/rebalance/cluster_status.cc
M src/kudu/rebalance/cluster_status.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
M src/kudu/tools/tool_action_cluster.cc
10 files changed, 582 insertions(+), 174 deletions(-)
Approvals:
Alexey Serbin: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@245
PS2, Line 245: for (const auto& str : categories_str) {
> Replaced with strings::Split(). Not sure I understand the 'embed it direct
What I meant is this:
for (const auto str : strings::Split(str, ...)) {
// Operate on 'str'.
}
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@722
PS2, Line 722: tservers_flags.end(),
> Nope, set_symmetric_difference needs elements to be ordered to operate corr
I don't know, and unless you can show a meaningful difference in a profiler, I wouldn't bother.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Mar 2020 21:39:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 5: Verified+1
unrelated test failure in org.apache.kudu.client.TestSplitKeyRange
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2020 05:33:52 +0000
Gerrit-HasComments: No
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@10
PS1, Line 10: Particularly, with this patch
: ksck issues a warning if different masters or different servers have
: different settings for the following flags:
: * --time_source
: * --builtin_ntp_servers
> Thank you for the feedback!
When ksck prints a warning, does it also exit with a non-zero status? If so, I'd be inclined not to warn here, as some folks may have good reason for the differing configuration.
If not, then let's keep it.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 20:33:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15298
to look at the new patch set (#4).
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
[ksck] report on misconfiguration for flag categories
This patch adds functionality to report on misconfiguration of flags
in pre-defined flag categories. The 'misconfiguration' means different
values for same flags in each group of masters/tablet servers in a
cluster (i.e. intra-group differences), and also differences between
masters' and tablet servers' flags (i.e. cross-group differences).
With this patch, two categories are defined and ready for use:
* 'time_source' includes the following flags
** --builtin_ntp_servers
** --time_source
* 'unusual' includes all experimental, hidden, and unsafe flags
The newly introduced functionality is enabled by default for the
'time_source' flags category. The list of group categories to check
can be customized by adding
--flags_categories_to_check=<comma-separated-list-of-flags-categories>
to the 'kudu cluster ksck' invocation. Use an empty string to specify
an empty list of flags categories to check.
For testing, I ran a small Kudu cluster and verified that ksck reports
on misconfiguration as expected:
* if tablet servers are run with different flags
* if masters are run with flag settings different from those used for
tablet servers
This patch also contains assorted re-factoring in related areas
of code. I'm planning to add automated tests to cover the newly
introduced functionality in a separate patch.
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
---
M src/kudu/rebalance/cluster_status.cc
M src/kudu/rebalance/cluster_status.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
M src/kudu/tools/tool_action_cluster.cc
10 files changed, 581 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/15298/4
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/1//COMMIT_MSG@10
PS1, Line 10: Particularly, with this patch
: ksck issues a warning if different masters or different servers have
: different settings for the following flags:
: * --time_source
: * --builtin_ntp_servers
> Why is it important that all masters or all tservers share the same values
Thank you for the feedback!
Using different time source for different might be a sign of misconfiguration. In theory, we can allow using built-in NTP client for some servers and rely on local synchronized clocks on others, but I'm not sure this is a viable use case. So, if those flags have different values, it most likely a sign of misconfiguration. Do you think it's too much to warn about such a case?
Yep, the verification that all masters _and_ tablet servers use the same source is in the making. I should have noted this as one of the WIP notes.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 20:28:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15298/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/15298/4/src/kudu/tools/ksck-test.cc@152
PS4, Line 152: cat < FlagsCategory::MAX
> Should this be
Done
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2020 04:40:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [ksck] report on misconfiguration of time source flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: WIP [ksck] report on misconfiguration of time source flags
......................................................................
Patch Set 1: Verified+1
unrelated test failure in org.apache.kudu.client.TestKuduSession
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 19:00:21 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@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-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15298
to look at the new patch set (#3).
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
[ksck] report on misconfiguration for flag categories
This patch adds functionality to report on misconfiguration of flags
in pre-defined flag categories. The 'misconfiguration' means different
values for same flags in each group of masters/tablet servers in a
cluster (i.e. intra-group differences), and also differences between
masters' and tablet servers' flags (i.e. cross-group differences).
With this patch, two categories are defined and ready for use:
* 'time_source' includes the following flags
** --builtin_ntp_servers
** --time_source
* 'unusual' includes all experimental, hidden, and unsafe flags
The newly introduced functionality is enabled by default for the
'time_source' flags category. The list of group categories to check
can be customized by adding
--flags_categories_to_check=<comma-separated-list-of-flags-categories>
to the 'kudu cluster ksck' invocation. Use an empty string to specify
an empty list of flags categories to check.
For testing, I ran a small Kudu cluster and verified that ksck reports
on misconfiguration as expected:
* if tablet servers are run with different flags
* if masters are run with flag settings different from those used for
tablet servers
This patch also contains assorted re-factoring in related areas
of code. I'm planning to add automated tests to cover the newly
introduced functionality in a separate patch.
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
---
M src/kudu/rebalance/cluster_status.cc
M src/kudu/rebalance/cluster_status.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
M src/kudu/tools/tool_action_cluster.cc
10 files changed, 583 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/15298/3
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15298
to look at the new patch set (#5).
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
[ksck] report on misconfiguration for flag categories
This patch adds functionality to report on misconfiguration of flags
in pre-defined flag categories. The 'misconfiguration' means different
values for same flags in each group of masters/tablet servers in a
cluster (i.e. intra-group differences), and also differences between
masters' and tablet servers' flags (i.e. cross-group differences).
With this patch, two categories are defined and ready for use:
* 'time_source' includes the following flags
** --builtin_ntp_servers
** --time_source
* 'unusual' includes all experimental, hidden, and unsafe flags
The newly introduced functionality is enabled by default for the
'time_source' flags category. The list of group categories to check
can be customized by adding
--flags_categories_to_check=<comma-separated-list-of-flags-categories>
to the 'kudu cluster ksck' invocation. Use an empty string to specify
an empty list of flags categories to check.
For testing, I ran a small Kudu cluster and verified that ksck reports
on misconfiguration as expected:
* if tablet servers are run with different flags
* if masters are run with flag settings different from those used for
tablet servers
This patch also contains assorted re-factoring in related areas
of code. I'm planning to add automated tests to cover the newly
introduced functionality in a separate patch.
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
---
M src/kudu/rebalance/cluster_status.cc
M src/kudu/rebalance/cluster_status.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
M src/kudu/tools/tool_action_cluster.cc
10 files changed, 582 insertions(+), 174 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/15298/5
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15298/4/src/kudu/tools/ksck-test.cc
File src/kudu/tools/ksck-test.cc:
http://gerrit.cloudera.org:8080/#/c/15298/4/src/kudu/tools/ksck-test.cc@152
PS4, Line 152: cat < FlagsCategory::MAX
Should this be
for (size_t cat = FlagsCategory::MIN; cat <= FlagsCategory::MAX; ++cat) {
? Though I suppose it doesn't matter much since this is a test.
Or define an ARRAY_SIZE = MAX + 1 like we do for protobuf? Might be worth doing given the current usages.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2020 04:01:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@12
PS2, Line 12: differences between masters' and tablet servers' settings
: for same flags.
> Isn't this redundant w.r.t. the first half of the sentence?
Rephrased in an attempt to clarify on that.
http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@21
PS2, Line 21: To use the newly introduced functionality, add
: --flags_categories_to_check=<comma-separated-list-of-flags-categories>
: to the 'kudu cluster ksck' invocation.
> Thoughts on enabling this by default?
Sure, we can do that: the very first version of this patch had this enabled. For some reason, after processing feedback from PS1 I was under impression we'd like to report on that only if requested explicitly. By after re-reading your comments I think we can enable this by default because
1) It could help spotting a misconfiguration
2) Warning don't lead to non-zero exit status anyways, so it's safe from the scripting/API compatibility point
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@57
PS2, Line 57: #define STR_FLAGS_CATEGORY_TIME_SOURCE "time_source"
: #define STR_FLAGS_CATEGORY_UNUSUAL "unusual"
> Could use C-string constants?
Using C-string constants would not allow for using those in the description of flags, unfortunately.
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@245
PS2, Line 245: SplitStringUsing(str, ",", &categories_str);
> I think we typically using strings::Split rather than this. You can embed i
Replaced with strings::Split(). Not sure I understand the 'embed it directly in the loop' part.
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@390
PS2, Line 390: void Ksck::AddFlagsToFlagMap(const GetFlagsResponsePB& flags,
: const string& server_address,
: KsckFlagToServersMap* flags_to_servers_map) {
: CHECK(flags_to_servers_map);
: for (const auto& f : flags.flags()) {
: const pair<string, string> key(f.name(), f.value());
: if (!InsertIfNotPresent(flags_to_servers_map, key, { server_address })) {
: FindOrDieNoPrint(*flags_to_servers_map, key).push_back(server_address);
: }
: }
: }
> Seems pretty straight-forward to reuse the existing AddFlagsToFlagMaps with
Indeed.
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@722
PS2, Line 722: set<KsckFlag> masters_flags;
> Could use unordered_sets here?
Nope, set_symmetric_difference needs elements to be ordered to operate correctly: https://en.cppreference.com/w/cpp/algorithm/set_symmetric_difference
Another option might be using std::vector, and then calling sort() && unique() before supplying to set_symmetric_difference(). That might bring a bit of performance gain in case larger flag categories. Is it worth it?
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@753
PS2, Line 753: // The flag/value pair must be either of masters' or tservers'.
: DCHECK(false);
> LOG(DFATAL) instead?
Done
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h
File src/kudu/tools/ksck_results.h:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h@174
PS2, Line 174: // Print a formatted summary of the flags in 'flag_to_servers_map',
: // indicating which servers have which (flag, value) pairs set.
> Could you reword this in terms of the existing PrintFlagTable, or vice vers
Done
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Mar 2020 19:13:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Volodymyr Verovkin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15298
to look at the new patch set (#2).
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
[ksck] report on misconfiguration for flag categories
This patch adds functionality to report on misconfiguration of flags
in pre-defined flag categories. The 'misconfiguration' means different
values for same flags among all masters and tablet servers in a Kudu
cluster, and differences between masters' and tablet servers' settings
for same flags.
With this patch, two categories are defined and ready for use:
* 'time_source' includes the following flags
** --builtin_ntp_servers
** --time_source
* 'unusual' includes all experimental, hidden, and unsafe flags
To use the newly introduced functionality, add
--flags_categories_to_check=<comma-separated-list-of-flags-categories>
to the 'kudu cluster ksck' invocation.
For testing, I ran a small Kudu cluster and verified that ksck reports
on misconfiguration as expected:
* if tablet servers are run with different flags
* if masters are run with flag settings different from those used for
tablet servers
This patch also contains assorted re-factoring in related areas
of code. I'm planning to add automated tests to cover the newly
introduced functionality in a separate patch.
Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
---
M src/kudu/rebalance/cluster_status.cc
M src/kudu/rebalance/cluster_status.h
M src/kudu/tools/ksck-test.cc
M src/kudu/tools/ksck.cc
M src/kudu/tools/ksck.h
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/ksck_remote.h
M src/kudu/tools/ksck_results.cc
M src/kudu/tools/ksck_results.h
M src/kudu/tools/tool_action_cluster.cc
10 files changed, 592 insertions(+), 165 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/15298/2
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15298/3/src/kudu/tools/ksck.h
File src/kudu/tools/ksck.h:
http://gerrit.cloudera.org:8080/#/c/15298/3/src/kudu/tools/ksck.h@183
PS3, Line 183: // Flags specific to the time source, clock, and alike.
: TIME_SOURCE = MIN,
:
: // Flags tagged hidden, experimental, or unsafe.
: UNUSUAL = MIN + 1,
nit: is it important that TIME_SOURCE will always be MIN? If not, maybe just define TIME_SOURCE and UNUSUAL as 0 and 1 respectively? Might be more quickly readable and more easily changeable in the future.
http://gerrit.cloudera.org:8080/#/c/15298/3/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/15298/3/src/kudu/tools/ksck.cc@65
PS3, Line 65: comma
nit: capitalize this
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Mar 2020 00:58:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 2: Verified+1
Unrelated test failure:
* AdminCliTest.TestLeaderTransferToEvictedPee
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Mar 2020 06:05:30 +0000
Gerrit-HasComments: No
[kudu-CR] [ksck] report on misconfiguration for flag categories
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc
File src/kudu/tools/ksck.cc:
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@245
PS2, Line 245: SplitStringUsing(str, ",", &categories_str);
> What I meant is this:
Ah, I see. That didn't work for me, so I'm leaving the temporary vector for now:
[ 84%] Building CXX object src/kudu/tools/CMakeFiles/ksck.dir/ksck.cc.o
src/kudu/tools/ksck.cc:246:19: error: no matching function for call to 'StringToFlagsCategory'
RETURN_NOT_OK(StringToFlagsCategory(str, &cat));
^~~~~~~~~~~~~~~~~~~~~
src/kudu/util/status.h:138:31: note: expanded from macro 'RETURN_NOT_OK'
#define RETURN_NOT_OK KUDU_RETURN_NOT_OK
^
src/kudu/util/status.h:36:33: note: expanded from macro 'KUDU_RETURN_NOT_OK'
const ::kudu::Status& _s = (s); \
^
src/kudu/tools/ksck.cc:225:8: note: candidate function not viable: no known conversion from 'const StringPiece' to 'const std::__1::string' (aka 'const basic_string<char, char_traits<char>, allocator<char> >') for 1st argument
Status StringToFlagsCategory(const string& str, FlagsCategory* category) {
^
1 error generated.
http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@722
PS2, Line 722: set<KsckFlag> masters_flags;
> I don't know, and unless you can show a meaningful difference in a profiler
OK, I think we can keep std::set() for a while then and look for alternatives when there is a need to optimize for performance for some particular scenarios.
--
To view, visit http://gerrit.cloudera.org:8080/15298
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb
Gerrit-Change-Number: 15298
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Mar 2020 22:56:29 +0000
Gerrit-HasComments: Yes