You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/07/24 19:04:53 UTC
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Adar Dembo, Hao Hao,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/11036
to review the following change.
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
4 files changed, 89 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/1
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#14).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
6 files changed, 157 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/14
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599
PS1, Line 599:
> Yes, by removing them as optional parameters they will not be shown in the
Thank you for the explanation. Yep, the reasoning makes sense to me.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 23:03:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2193
PS1, Line 2193: master_addr
> nit: master_addrs. And also in all other places.
Pretty much all the test cases in this file are using 'master_addr', I think because the tests aren't configured to with multi-master.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211
PS1, Line 2211: Kerberos is enabled
> Do we need to parameterized this with Kerberos disabled/enabled?
I opted to have this one be kerberized and TestCheckAndManualFixHmsMetadata not be kerberized. I think that will give us enough coverage without having to execute the test-cases twice.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h@115
PS1, Line 115: // explicitly set.
> Can you add a bit explanation/comment on what is the parameter for? And how
Done
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599
PS1, Line 599:
> The comment for the changelist says something like '... when it is not prov
Yes, by removing them as optional parameters they will not be shown in the output of 'kudu hms check --help', however they are still settable. In effect, they are 'hidden'. I'd prefer them to not be shown, because the vast majority of the time they should be looked up on the master. The fallback is in place in case a different HMS address must be used, but I can't imagine at this point why you'd want to do that.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113
PS1, Line 113: FLAGS_hive_metastore_uris.empty()
> When the URIs and SASL flags will not be empty? Do we need to validate if t
hive_metastore_uris won't be empty if it's set on the command line via a flag. In that case it's used instead of the master config because the user is setting it specifically.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master_addrs[0]
> Is it possible that different masters have different configs?
Eventually I think we should add a check that all masters have the same configuration value, but I think that would better be handled as it's own check as a part of ksck or 'hms check'.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master::Master::kDefaultPort
> What happens if the port being used is different from the default?
The provided port is the fallback in case the master address doesn't include a port.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120
PS1, Line 120: hive_metastore_uris
> nit: does it make sense to introduce constant literals for 'hive_metastore
These are derivative from the associated flag definitions. Ideally there would be a way to get the flag name from the flag symbol (FLAGS_hive_metastore_uris -> 'hive_metastore_uris'), so that if the flag name ever changes this code would stop compiling. I can't figure out a way to do that, though, and I don't think there's a lot of value in defining a constant otherwise, since it isn't the definitive version.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:15:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#4).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
4 files changed, 99 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/4
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 6:
One thing I noticed today: when using the hms check tool and passing 'localhost' as the master addresses it will put that into the HMS. We should instead look at the master addresses config on the master to get the canonical version.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 27 Jul 2018 01:18:13 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#11).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
5 files changed, 153 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/11
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc@2230
PS13, Line 2230: hms downgrade $0", master_addr
nit: Would you mind adding a comment to say that this is to test without providing hive_metastore_uris and hive_metastore_sasl_enabled via command line flags, it also works.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 18:27:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#13).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
6 files changed, 155 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/13
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#3).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
4 files changed, 97 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/3
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 25 Jul 2018 01:19:32 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 11:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc@2215
PS10, Line 2215: NO_FATALS(StartExternalMiniCluster(std::move(opts)));
> Maybe parameterize this instead so we don't lose non-kerberized coverage? M
Done. On my machine none of the shards take more than 60s so I didn't increase it from 4.
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h@114
PS10, Line 114: // 'all_flags' controls whether all flags are returned, or only flags which are
: // explicitly set.
> Can you define a new enum for this instead?
This option follows directly from the protobuf interface[1] which this method directly calls, so I'd prefer not to change it. It's also used as a bool through the all_flags flag[2].
[1]: https://github.com/apache/kudu/blob/master/src/kudu/server/server_base.proto#L45
[2]: https://github.com/apache/kudu/blob/master/src/kudu/tools/tool_action_common.cc#L101
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@114
PS10, Line 114: if (FLAGS_hive_metastore_uris.empty()) {
> See below: since --hive-metastore-uris isn't advertised anymore, we should
It's now been added to the optional arg list.
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@123
PS10, Line 123:
> Is this even possible?
Done
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@137
PS10, Line 137: FLAGS_hive_metastore_uris = hms_uris->value();
: // SetCommandLineOption avoids needing to parse the value into a bool.
: CHECK(!gflags::SetCommandLineOption("hive_metastore_sasl_enabled",
> Why are these set via different methods? I can take my answer in the form o
Done
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@143
PS10, Line 143: // Create HMS catalog.
> As a TODO item, it would be nice if HmsCatalog (and maybe HmsClient) behave
I agree in principal, but I think it's pretty nice that the low level timeout and buffer size flags are not exposed in the tool help output, but can still be overridden in exceptional circumstances.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Jul 2018 23:26:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed a vote on this change.
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Reviewed-on: http://gerrit.cloudera.org:8080/11036
Reviewed-by: Hao Hao <ha...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
6 files changed, 157 insertions(+), 64 deletions(-)
Approvals:
Hao Hao: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 15
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 18:35:43 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 13:
(2 comments)
> Patch Set 12:
>
> (2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc@2247
PS12, Line 2247: NO_FATALS(RunAction
> wrap into NO_FATALS()?
Done
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc@2248
PS12, Line 2248: NO_FATALS(RunAction
> wrap into NO_FATALS()?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 02:06:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 7:
> Patch Set 6:
>
> One thing I noticed today: when using the hms check tool and passing 'localhost' as the master addresses it will put that into the HMS. We should instead look at the master addresses config on the master to get the canonical version.
I'm going to address this in a follow up patch.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 27 Jul 2018 22:17:40 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 10: Verified+1
unrelated flake
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Jul 2018 17:10:34 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599
PS1, Line 599:
The comment for the changelist says something like '... when it is not provided via command line flags ...'. Is that intended that those flags became hidden once they are removed from the list of optional flags for the HMS-related CLI actions?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120
PS1, Line 120: hive_metastore_uris
nit: does it make sense to introduce constant literals for 'hive_metastore_uris' and 'hive_metastore_sasl_enabled' flag names?
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 21:36:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211
PS1, Line 2211: Kerberos is enabled
> I opted to have this one be kerberized and TestCheckAndManualFixHmsMetadata
Makes sense.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113
PS1, Line 113: FLAGS_hive_metastore_uris.empty()
> hive_metastore_uris won't be empty if it's set on the command line via a fl
If the flag is set by the command line but is different from the one specified in master, should log a warning so that users knows what he is doing in this case?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master_addrs[0]
> Eventually I think we should add a check that all masters have the same con
Sounds reasonable to me. Should we add a TODO for it?
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:39:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 13: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc@135
PS12, Line 135: // SetCommandLineOption avoids needing
> ah so the latest revision introduced a bug here that I didn't realize till
Ah, indeed. Good catch!
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 02:16:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#2).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
4 files changed, 95 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/2
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Jul 2018 23:44:32 +0000
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc@135
PS12, Line 135: CHECK(hms_sasl_enabled != flags.end());
Just curious: why it's not an option to have DCHECK() instead of CHECK() here and returning Status::ConfigurationError() or Status::IllegalState() if hms_sasl_enabled?
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc@692
PS12, Line 692: .AddOptionalParameter("hive_metastore_sasl_enabled", boost::none, kHmsSaslEnabledDesc)
: .AddOptionalParameter("hive_metastore_uris", boost::none, kHmsUrisDesc)
It might be mentioned elsewhere, but it seems I missed that and could not find the answer: why these parameters are present for 'fix' and 'downgrade' sub-commands but absent in the 'check' sub-command? Could you clarify?
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 01:05:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#12).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
5 files changed, 154 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/12
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11036
to look at the new patch set (#5).
Change subject: hms tools: do not require HMS configuration flags
......................................................................
hms tools: do not require HMS configuration flags
The HMS tools will now lookup required HMS configuration
(hive_metastore_uris and hive_metastore_sasl_enabled) from the master
when it is not provided via command line flags. Looking up the configs
on the master is more ergonomic and less error-prone.
Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_hms.cc
4 files changed, 101 insertions(+), 54 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/5
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/kudu-tool-test.cc@2215
PS10, Line 2215: // Kerberos is enabled in order to test the tools work in secure clusters.
Maybe parameterize this instead so we don't lose non-kerberized coverage? May need to increase the number of shards too.
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_common.h@114
PS10, Line 114: // 'all_flags' controls whether all flags are returned, or only flags which are
: // explicitly set.
Can you define a new enum for this instead?
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@114
PS10, Line 114: if (FLAGS_hive_metastore_uris.empty()) {
See below: since --hive-metastore-uris isn't advertised anymore, we should assume that this is always true.
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@123
PS10, Line 123: hms_uris->value().empty()
Is this even possible?
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@137
PS10, Line 137: FLAGS_hive_metastore_uris = hms_uris->value();
: CHECK(!gflags::SetCommandLineOption("hive_metastore_sasl_enabled",
: hms_sasl_enabled->value().c_str()).empty());
Why are these set via different methods? I can take my answer in the form of a code comment.
http://gerrit.cloudera.org:8080/#/c/11036/10/src/kudu/tools/tool_action_hms.cc@143
PS10, Line 143: hms_catalog->reset(new hms::HmsCatalog(master_addrs_flag));
As a TODO item, it would be nice if HmsCatalog (and maybe HmsClient) behaved more like libraries and received configurable options via constructor rather than via gflags. Without that, I think it's still possible for a CLI user to set --hive-metastore-uris on the command line and for that to take effect, which doesn't make sense because that gflag is no longer advertised via AddOptionalParameter.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Jul 2018 17:38:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/13/src/kudu/tools/kudu-tool-test.cc@2230
PS13, Line 2230: ore_sasl_enabled are automatic
> nit: Would you mind adding a comment to say that this is to test without pr
Done
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 18:34:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2193
PS1, Line 2193: master_addr
nit: master_addrs. And also in all other places.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211
PS1, Line 2211: Kerberos is enabled
Do we need to parameterized this with Kerberos disabled/enabled?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h@115
PS1, Line 115: bool all_flags,
Can you add a bit explanation/comment on what is the parameter for? And how it would work with 'flag_tags'?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113
PS1, Line 113: FLAGS_hive_metastore_uris.empty()
When the URIs and SASL flags will not be empty? Do we need to validate if the flags' value match with the ones in master in this case?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master_addrs[0]
Is it possible that different masters have different configs?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master::Master::kDefaultPort
What happens if the port being used is different from the default?
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120
PS1, Line 120: hive_metastore_uris
> nit: does it make sense to introduce constant literals for 'hive_metastore
+1
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 21:52:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113
PS1, Line 113: FLAGS_hive_metastore_uris.empty()
> If the flag is set by the command line but is different from the one specif
I think i'd rather keep this simple and skip looking up the config from the master in that case. It should be an exceptional circumstance that the flag gets set manually, and I don't expect it to happen accidentally because its no longer listed as an optional flag in the help output.
http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116
PS1, Line 116: master_addrs[0]
> Sounds reasonable to me. Should we add a TODO for it?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 23:01:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc@2247
PS12, Line 2247: RunActionStdoutNone
wrap into NO_FATALS()?
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/kudu-tool-test.cc@2248
PS12, Line 2248: RunActionStdoutNone
wrap into NO_FATALS()?
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 01:18:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 )
Change subject: hms tools: do not require HMS configuration flags
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc@135
PS12, Line 135: CHECK(hms_sasl_enabled != flags.end());
> Just curious: why it's not an option to have DCHECK() instead of CHECK() he
ah so the latest revision introduced a bug here that I didn't realize till you pointed this section out. The original reason was that we were retrieving all the flags from the master, and if the hive_metastore_uris flag exists on the master then the hive_metastore_sasl_enabled flag must also, however now we're only retrieving non-default flags, so we should just assum false if it's not in the list.
http://gerrit.cloudera.org:8080/#/c/11036/12/src/kudu/tools/tool_action_hms.cc@692
PS12, Line 692: .AddOptionalParameter("hive_metastore_sasl_enabled", boost::none, kHmsSaslEnabledDesc)
: .AddOptionalParameter("hive_metastore_uris", boost::none, kHmsUrisDesc)
> It might be mentioned elsewhere, but it seems I missed that and could not f
Woops, they should be present for check as well.
--
To view, visit http://gerrit.cloudera.org:8080/11036
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1
Gerrit-Change-Number: 11036
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 01 Aug 2018 01:13:03 +0000
Gerrit-HasComments: Yes