You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/06/04 17:53:29 UTC

[kudu-CR] hms: allow for tooling to run without Kudu plugin 

Hello Alexey Serbin, Grant Henke, Hao Hao,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/13510

to review the following change.


Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
13 files changed, 102 insertions(+), 44 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/1
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@27
PS2, Line 27: kserver
nit: tserver?


http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@31
PS2, Line 31: an
nit: a


http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@81
PS2, Line 81: configured to listen to Kudu
I think this comment is a bit misleading. If the only difference is to enabled Kudu plugin. Could you rephrase it to 'The HMS will be started and configured to use Kudu plugin,...' ?


http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@85
PS2, Line 85: configured to listen to Kudu
Same here.


http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/hms/mini_hms.h@63
PS2, Line 63: EnableKuduListener
nit: rename to EnableKuduPlugin?



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 05 Jun 2019 00:40:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208
PS3, Line 208: is_server
This looks good and generic, but as a second thought, I'm curious whether a targeted approach will be a better case here.

I.e., what if we set/register the group validator only for the master's code?  Something like moving GROUP_FLAG_VALIDATOR(hive_metastore_sasl_enabled, ValidateHiveMetastoreSaslEnabled); into master_main.cc or moving the whole group validator code there?



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 18:28:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13510

to look at the new patch set (#3).

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab flag if not using a kserver. Tool users
should just be able to kinit to authenticate. To get this behavior, I've
gated gflag validator on a new hidden gflag.

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
15 files changed, 109 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/3
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
I don't know the answer, but it seems like there should be a more exact way to check if running in a master or tserver. This could be surprising down the road if configuration expectations change.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Jun 2019 20:24:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@27
PS2, Line 27: server.
> nit: tserver?
Done


http://gerrit.cloudera.org:8080/#/c/13510/2//COMMIT_MSG@31
PS2, Line 31: 
> nit: a
Reading this out loud, this reads more naturally.


http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@81
PS2, Line 81: 
> I think this comment is a bit misleading. If the only difference is to enab
Done


http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/common/common.proto@85
PS2, Line 85: 
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208
PS3, Line 208: is_server
> This looks good and generic, but as a second thought, I'm curious whether a
I'm leaving this as is unless you feel strongly about it.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 20:24:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208
PS3, Line 208: is_server
> I'm leaving this as is unless you feel strongly about it.
OK, it's up to you -- I don't feel strong about that.  However, if it's not a big deal just to move that piece of code into master_main.cc, we could avoid adding this artificial hidden 'is_server' flag.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:06:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13510

to look at the new patch set (#5).

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab_file flag if not running kudu-master. To get
this behavior, I've moved the gflag validator into master_main.cc, which
is not built by tooling. I manually tested that it works, i.e. that
tooling will not validate and that a master will.

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/master/master_main.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
14 files changed, 123 insertions(+), 63 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/5
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> This caught my eye as well. A hack might be to add a hidden gflag whose def
+1 for the CLI-specific macro and #define that uses it.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 05 Jun 2019 18:17:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:56:07 +0000
Gerrit-HasComments: No

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> I don't know the answer, but it seems like there should be a more exact way
This caught my eye as well. A hack might be to add a hidden gflag whose default value is different in the CLI vs. elsewhere, and condition on that gflag's value here.

Another hack would be to introduce a #define, to condition on it here, and to have it set by CMake only when building the CLI.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Jun 2019 20:30:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208
PS3, Line 208: 
> Seems I  misunderstood this at first. Seems to work!
Great!  Thank you for addressing this.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:55:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> After discussing it offline with Andrew, it seems the idea with a higgen gf
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 18:00:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

http://gerrit.cloudera.org:8080/#/c/13510/3/src/kudu/server/server_base.cc@208
PS3, Line 208: 
> OK, it's up to you -- I don't feel strong about that.  However, if it's not
Seems I  misunderstood this at first. Seems to work!

Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:36:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> I'm not sure this will work -- as I understand, master.cc is compiled as a 
Ah, yeah, you're right. You'd have to compile master.cc twice for this to work.

OK, I get the motivation for a gflag now.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:29:49 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 21:36:07 +0000
Gerrit-HasComments: No

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> Done
Hmm, why is a separate source file necessary? Wouldn't this work?

  bool ValidateHiveMetastoreSaslEnabled() {
  #ifndef KUDU_IN_CLI
    if (FLAGS_hive_metastore_sasl_enabled &&
        FLAGS_keytab_file.empty()) {
      return false;
    }
  #endif
    return true;
  }

Then in tools/CMakeLists.txt:

  add_compile_definitions(KUDU_IN_CLI);



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 18:43:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13510

to look at the new patch set (#4).

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab flag if not using a server. Tool users
should just be able to kinit to authenticate. To get this behavior, I've
gated gflag validator on a new hidden gflag and manually tested that it
works (i.e. that tooling will not validate and that a server will).

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_main.cc
15 files changed, 109 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13510

to look at the new patch set (#2).

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab flag if not using a kserver. Tool users
should just be able to kinit to authenticate. To get this behavior, I've
gated gflag validator on the stable -fs_wal_dir required flag.

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
13 files changed, 103 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/2
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> +1 for the CLI-specific macro and #define that uses it.
After discussing it offline with Andrew, it seems the idea with a higgen gflag is a better fit.

In case of 'macro approach' it would be necessary to create a separate source .cc file that we compile with different compiler flags for the 'kudu' and the 'master' binaries, linking the result object files into corresponding binaries.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 17:44:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab_file flag if not running kudu-master. To get
this behavior, I've moved the gflag validator into master_main.cc, which
is not built by tooling. I manually tested that it works, i.e. that
tooling will not validate and that a master will.

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Reviewed-on: http://gerrit.cloudera.org:8080/13510
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/master/master_main.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
15 files changed, 121 insertions(+), 65 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13510

to look at the new patch set (#6).

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................

hms: allow for tooling to run without Kudu plugin

Currently, whenever an HMS client successfully connects to the HMS, it
checks the various HMS service configurations (e.g. that it is using the
Kudu-HMS plugin), returning an error if any are misconfigured. This is
important to make it much more obvious when the Kudu Master's HMS
synchronization is misconfigured.

It is still useful for other HMS clients (e.g. that used by the HMS
tooling) to operate on an HMS instance that is not configured with the
Kudu-HMS plugin et al.

This patch removes the requirement by plumbing the option to the HMS
client as a member of ThriftOptions. This was the most straightforward
way to plumb this option from the HmsCatalog to the HmsClient, given the
templating layered in between them for HA. Besides, we can use this
option in the future if we ever want to verify the configuration of
Thrift-based clients for other services (e.g. Sentry).

This patch additionally allows the -hive_metastore_sasl_enabled flag to
be used without the -keytab_file flag if not running kudu-master. To get
this behavior, I've moved the gflag validator into master_main.cc, which
is not built by tooling. I manually tested that it works, i.e. that
tooling will not validate and that a master will.

To test, I added an HmsMode that starts the HMS without the Kudu-HMS
plugin installed and used it in a couple of HMS tooling tests. I
considered reusing the ENABLE_HIVE_METASTORE HmsMode, but opted not to
since some tests are greatly simplified by ENABLE_HIVE_METASTORE having
the Kudu-HMS plugin installed (e.g. restarting the HMS isn't required to
enable the Kudu-HMS integration).

Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
---
M src/kudu/common/common.proto
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.cc
M src/kudu/master/master_main.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/server_base.cc
M src/kudu/thrift/client.h
M src/kudu/tools/kudu-tool-test.cc
15 files changed, 121 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13510/6
-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2: Verified+1

TSAN: 	org.apache.kudu.flume.sink.SecureKuduSinkTest


-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 Jun 2019 20:17:58 +0000
Gerrit-HasComments: No

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/13510/2/src/kudu/master/master.cc@123
PS2, Line 123: // Note: this check only needs to be run on a server. E.g. tools that run with
> Hmm, why is a separate source file necessary? Wouldn't this work?
I'm not sure this will work -- as I understand, master.cc is compiled as a part of the master library, so the compiler is run only once for this source file, right?

At least, I can see from compile_commands.json that the compiler is invoked only once for this master.cc file.  Maybe be I'm missing something.



-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 18:47:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] hms: allow for tooling to run without Kudu plugin

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13510 )

Change subject: hms: allow for tooling to run without Kudu plugin
......................................................................


Patch Set 6: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/13510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b9968bf0f8a55859a14421beda05cab3496b6c0
Gerrit-Change-Number: 13510
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@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: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 06 Jun 2019 22:56:22 +0000
Gerrit-HasComments: No