subject:"\[kudu\-CR\] KUDU\-2191 \(12\/n\)\: Hive Metastore notification log event listener"

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 37: Code-Review+2

Woo hoo.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 37
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 15 Jun 2018 04:52:46 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 37:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc
File src/kudu/master/hms_notification_log_listener-test.cc:

http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc@33
PS35, Line 33: 
DECLARE_uint32(hive_metastore_notification_log_poll_inject_latency_ms);
> Are these timing tests robust? Do they pass when looped in TSAN mode with s
Yes.  All of the waits are just to tickle out interesting code paths in the 
implementation; but in all cases the tests should pass with arbitrary delays 
(or not).  I've looped under TSAN with 0 and 8 stress threads and seen no 
failures.


http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc@35
PS35, Line 35: pace kudu {
> I think we like to have test fixtures inherit from KuduTest even if they do
I've added the test fixture, but not the creation of the listener because I 
want the flags to be set before starting the task thread.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 37
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 15 Jun 2018 01:34:35 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener-test.cc
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
19 files changed, 1,266 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/37
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 37
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-14 Thread Hao Hao (Code Review)

Hao Hao has uploaded a new patch set (#36) to the change originally created by 
Dan Burkert. ( http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,129 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/36
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

Patch Set 35:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc
File src/kudu/master/hms_notification_log_listener-test.cc:

http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc@33
PS35, Line 33:
Are these timing tests robust? Do they pass when looped in TSAN mode with some
stress threads?

http://gerrit.cloudera.org:8080/#/c/8313/35/src/kudu/master/hms_notification_log_listener-test.cc@35
PS35, Line 35: HmsNotificationLogListenerTest
I think we like to have test fixtures inherit from KuduTest even if they don't
need any functionality there. Can't remember why though.

That said, it does look like there's some commonality that could be stored in a
test fixture. The creation of the listener, for one. And poll_period too.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 35
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:55:34 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener-test.cc
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
19 files changed, 1,262 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/35
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 35
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 35:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/34/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/34/src/kudu/master/hms_notification_log_listener.cc@126
PS34, Line 126: }
> Missing an std::move() here, no?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 35
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:48:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 34:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/34/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/34/src/kudu/master/hms_notification_log_listener.cc@126
PS34, Line 126: catch_up_callbacks = catch_up_callbacks_;
Missing an std::move() here, no?

BTW, may want to rename the local variable: catch_up_callbacks and 
catch_up_callbacks_ are almost the same.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 34
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 21:25:57 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 34:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc@126
PS33, Line 126: catch_up_callbacks = catch_up_callbacks_;
> Generally speaking you may not want to run these with lock_ held. Granted,
Done


http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc@130
PS33, Line 130: "Hive Metastore notification log listener is 
shutting down"));
> The HmsNotificationLogListener is probably on its way toward being destroye
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 34
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 21:22:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,129 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/34
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 34
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 33:

(2 comments)

Just a nit or two.

http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc@126
PS33, Line 126: for (auto& cb : catch_up_callbacks_) {
Generally speaking you may not want to run these with lock_ held. Granted, 
these all just set a Status and tickle a waiter so it's not a big deal.


http://gerrit.cloudera.org:8080/#/c/8313/33/src/kudu/master/hms_notification_log_listener.cc@130
PS33, Line 130: break;
The HmsNotificationLogListener is probably on its way toward being destroyed 
here, but maybe we should clear catch_up_callbacks_ anyway, to preserve the 
invariant that once run, a callback is removed from that list?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 33
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 21:14:06 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,127 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/33
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 33
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 32:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.h
File src/kudu/master/hms_notification_log_listener.h:

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.h@129
PS31, Line 129:   // Protects access to fields below.
> Need not be atomic anymore since it's protected by lock_. Should update the
Done


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@94
PS31, Line 94: Status HmsNotificationLogListenerTask::WaitForCatchUp(const 
MonoTime& deadline) {
> Should be protected by lock_ for consistency.
Done


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@115
PS31, Line 115:
  : // Wakeup all threads which enqueued before beginning the 
poll.
  : for (auto& cb : catch_up_callbacks) {
  :
> If a thread calls WaitForCatchUp() before the very first Poll(), it'll actu
As I understood the pseudocode was:

  while (true) {
{
  std::lock_guard l(lock_);
  catch_up_callbacks = std::move(catch_up_callbacks_);
  if (closing_) break;
}
Status s = Poll();
WARN_NOT_OK(s, "Hive Metastore notification log listener poll failed");
// Wakeup all threads waiting for catch up.
for (auto& cb : catch_up_callbacks) {
  cb.Run(s);
}
{
  std::lock_guard l(lock_);
  if (closing_) break;
  wake_up_cv_.WaitFor(

MonoDelta::FromSeconds(FLAGS_hive_metastore_notification_log_poll_period_seconds));
}
  }

As far as I can tell this has the same issue.  I've added a version which I 
don't think forces waiter threads to wait out a timeout under any circumstances.


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@210
PS31, Line 210: events.clear();
> Should be protected by lock_.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 32
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 19:41:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,127 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/32
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 32
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 31:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.h
File src/kudu/master/hms_notification_log_listener.h:

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.h@129
PS31, Line 129:   std::atomic_bool closing_;
Need not be atomic anymore since it's protected by lock_. Should update the 
comments here to reflect that too.


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@94
PS31, Line 94:   DCHECK(!closing_);
Should be protected by lock_ for consistency.


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@115
PS31, Line 115: // Wakeup all threads waiting for catch up.
  : for (auto& cb : catch_up_callbacks) {
  :   cb.Run(s);
  : }
If a thread calls WaitForCatchUp() before the very first Poll(), it'll actually 
have to wait for two Poll() calls and one WaitFor() before being signaled. 
That's unfortunate but livable I guess. I don't think the pseudocode I wrote a 
couple reviews ago had this problem though; why not just use that?


http://gerrit.cloudera.org:8080/#/c/8313/31/src/kudu/master/hms_notification_log_listener.cc@210
PS31, Line 210: if (closing_) {
Should be protected by lock_.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 18:50:22 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 31:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@124
PS28, Line 124:   if (closing_) break;
Thanks for this feedback, I was under the mistaken understanding that wakeups 
while no thread is waiting would be queued, but after reading the manpage I see 
that's false:

> The pthread_cond_broadcast() and pthread_cond_signal() functions shall have 
> no effect if there are no threads currently blocked on cond.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 18:11:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,120 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/31
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 30:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@124
PS28, Line 124: WARN_NOT_OK(s, "Hive Metastore notification log listener 
poll failed");
> Done
Unfortunately I think the new code is still vulnerable to this problem: T1 sets 
closing_, acquires lock_, signals CV, and releases lock_, then T2 acquires 
lock_, and waits on the CV. The disadvantage of this new iteration is also that 
the time to first poll is a full period instead of immediately, which may slow 
down tests.

I don't see a fix for this that doesn't involve combining the action on 
closing_ (either setting or checking) with the action on the CV (either 
signalling or waiting).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 04:08:14 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 30:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@124
PS28, Line 124: WARN_NOT_OK(s, "Hive Metastore notification log listener 
poll failed");
> This still isn't quite right: it's possible for Shutdown() and RunLoop() to
Done


http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@245
PS28, Line 245:   // table never existed in Kudu. It's critical that in 
cases like this
> Nit: receiving
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 01:14:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,113 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/30
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,113 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/29
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 27:

> Patch Set 27:
>
> (4 comments)
>
> > > Increasing HMS integration test coverage by parameterizing existing tests 
> > > the way you did makes sense, but do any of these tests now need to be 
> > > sharded? Are any approaching the 900s runtime mark, at which point they'd 
> > > be killed?
> >
> > master-stress-test is time boxed at 30s, so I'm going to leave that as is.  
> > I've shareded alter_table-randomized, though.
>
> What kind of change did you see to alter_table-randomized-test's running time 
> to warrant the sharding?
>
> Also, what about master_failover-itest?

Without sharding, alter_table_randomized-test went from 39 seconds to 76 
seconds (47 seconds of which is the HMS case).

Without sharding, master_failover-itest went from 206 seconds to 346 seconds.

I'll add sharding to both.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 14 Jun 2018 00:27:08 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 28:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@124
PS28, Line 124: std::lock_guard l(lock_);
This still isn't quite right: it's possible for Shutdown() and RunLoop() to 
interleave such that the CV signal is missed and another poll period must 
elapse:

  T1 (Shutdown)T2 (RunLoop)
   checks closing_
   closing_ is false, does not break
  sets closing_
  acquires lock_
  signals CV
  releases lock_
   acquires lock_
   releases lock_ and waits on CV

I think protecting closing_  with lock_ should fix this (and then you can make 
closing_ a simple bool). Something like:

  for (;;) {
take lock_
swap callbacks
break if closing_
release lock_
poll
run callbacks
take lock_
break if closing_
wait on CV (and release lock_)
  }

Not sure how to get rid of one of the closing_ checks though.


http://gerrit.cloudera.org:8080/#/c/8313/28/src/kudu/master/hms_notification_log_listener.cc@245
PS28, Line 245:   // TODO(KUDU-2475): Ignoring errors could result in a 
client receive an
Nit: receiving



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 28
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Jun 2018 23:26:32 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 28:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@1745
PS27, Line 1745: // renamed in the HMS.
> Nit: actually, here "lower" was correct, because the sentence was "this mak
Done


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@1748
PS27, Line 1748: // Look up and lock the table.
> We're not actually marking the table as removed; that's done later in respo
Done


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@2133
PS27, Line 2133:   // alteration to a table which has just been renamed or 
deleted through the HMS.
> Same.
Done


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc@129
PS27, Line 129:
> So if we wake up because Shutdown() was called, we'll still do one more Pol
Done


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc@229
PS27, Line 229:   }
  :
  :   // Failing to properly handle a notification is not a 
fatal error, instead
  :   // we continue processing notifications. Callers of 
WaitForCatchUp have no
  :   // way of indicating which specific notification they are 
waiting for, and
  :   // returning early with error pertaining to
> We talked about this on Slack.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 28
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Jun 2018 23:09:29 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,106 insertions(+), 222 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/28
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 28
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

Patch Set 27:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc@229
PS27, Line 229:
: // Failing to properly handle a notification is not a
fatal error, instead
: // we continue processing notifications. Callers of
WaitForCatchUp have no
: // way of indicating which specific notification they are
waiting for, and
: // returning early with error pertaining to a different
notifications
: // could result in not waiting long enough.
We talked about this on Slack.

Not being able to correlate notification log events to Kudu DDLs hurts
robustness. It means that a Kudu client might hear back OK for a DDL that
succeeded in HMS but failed in Kudu. Failures that cause the leader to lose
leadership will be properly reported back, and of course failures that lead to
crashes will be noticed, but all other failures will be silently reported as
successes. Brainstorming a bit, one such failure may be if we've got three
masters and the two follower masters die. I don't think the leader will detect
this and abdicate, but attempts to write to the system catalog will still fail.

This is a tough problem, so I don't think it should gate this patch, but please
note it in a TODO and file a JIRA. I'd like it to be addressed before we
advertise HMS integration as supported, though.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Jun 2018 21:17:59 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 27:

(4 comments)

> > Increasing HMS integration test coverage by parameterizing existing tests 
> > the way you did makes sense, but do any of these tests now need to be 
> > sharded? Are any approaching the 900s runtime mark, at which point they'd 
> > be killed?
>
> master-stress-test is time boxed at 30s, so I'm going to leave that as is.  
> I've shareded alter_table-randomized, though.

What kind of change did you see to alter_table-randomized-test's running time 
to warrant the sharding?

Also, what about master_failover-itest?

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@1745
PS27, Line 1745: // renamed in the HMS.
Nit: actually, here "lower" was correct, because the sentence was "this makes 
the likelihood of  lower" rather than "this reduces the likelihood of 
 [lower]".


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@1748
PS27, Line 1748: // Look up the table, lock it, and mark it as removed.
We're not actually marking the table as removed; that's done later in response 
to the HMS event.


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/catalog_manager.cc@2133
PS27, Line 2133:   // to a table which has just been renamed or deleted through 
the HMS.
Same.


http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/27/src/kudu/master/hms_notification_log_listener.cc@129
PS27, Line 129: wake_up_cv_.WaitFor(poll_period);
So if we wake up because Shutdown() was called, we'll still do one more Poll() 
before exiting. That doesn't seem right; shouldn't we just wake up threads 
stuck in WaitForCatchUp and then exit?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 13 Jun 2018 20:55:02 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace.

As a result, rename and delete RPCs are handled specially to ensure they
are applied to the HMS first, and only once they are committed in the
HMS are they applied to the Kudu catalog, through the new notification
log listener. Alter table operations which include a table rename and
other changes are handled by first performing the rename, then
performing the remaining changes. As a result, the alter table operation
as a whole is no longer applied atomically.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
18 files changed, 1,101 insertions(+), 222 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/27
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 26:

(12 comments)

> Patch Set 26:
>
> (12 comments)
>
> Increasing HMS integration test coverage by parameterizing existing tests the 
> way you did makes sense, but do any of these tests now need to be sharded? 
> Are any approaching the 900s runtime mark, at which point they'd be killed?

master-stress-test is time boxed at 30s, so I'm going to leave that as is.  
I've shareded alter_table-randomized, though.

http://gerrit.cloudera.org:8080/#/c/8313/26//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8313/26//COMMIT_MSG@16
PS26, Line 16:  As a result, rename and delete RPCs are handled specially to
 : ensure they are applied to the HMS first, and only once they are
 : committed in the HMS are they applied to the Kudu catalog, 
through the
 : new notification log listener.
> I would also call out the extra complexity of compound alters that rename t
Done


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/hms/hms_catalog.cc@440
PS26, Line 440:   // Set the table type to external so that the table's (HD)FS 
directory will
> So there's no way to get the HMS to just not create HDFS directories for Ku
nope :(


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master-stress-test.cc@a160
PS26, Line 160:
> I remember a discussion about this on Slack, but I thought you wouldn't nee
Here's a recap of the conversation on Slack for posterity:

This patch changes the CatalogManager to wait for the local notification log 
listener to apply the latest events before proceeding with DDL operations (see 
the comments in CatalogManager for more color).  Since this wait needs to be 
bounded by _some_ timeout, I chose to use the RPC's deadline.  It appears that 
this is the first use of the RPC deadline while handling DDL operations in the 
catalog manager.

Separately, we've had a long-standing hack in the C++ client here: 
https://github.com/apache/kudu/blob/1.7.1/src/kudu/client/client-internal.cc?utf8=%E2%9C%93#L183-L189.
  In effect, we set the RPC deadline to min(admin_timeout, rpc_timeout).

Since this test is setting rpc_timeout to 1s, the effect was that the catalog 
manager only waited 1s for the notification log listener to catch up.  This was 
causing many timeouts, since it can frequently take up to a few seconds in 
heavily loaded clusters.

However, it appears that setting the RPC timeout to a low value is not 
necessary for the client to quickly switch to alternate masters in the case of 
failover. I validated this by running the test with and without the 1s 
rpc_timeout (both times with HMS integration disabled):

This commit, HMS disabled, 1s rpc_timeout:

I0613 12:06:04.978294 86596 master-stress-test.cc:474] Tables created: 1024
I0613 12:06:04.978309 86596 master-stress-test.cc:475] Tables altered: 511
I0613 12:06:04.978315 86596 master-stress-test.cc:476] Tables deleted: 500
I0613 12:06:04.978338 86596 master-stress-test.cc:477] Tablets replaced: 465
I0613 12:06:04.978379 86596 master-stress-test.cc:478] Masters restarted: 59

This commit, HMS disabled:

I0613 12:02:02.931084 52591 master-stress-test.cc:480] Tables created: 979
I0613 12:02:02.931099 52591 master-stress-test.cc:481] Tables altered: 489
I0613 12:02:02.931103 52591 master-stress-test.cc:482] Tables deleted: 487
I0613 12:02:02.931125 52591 master-stress-test.cc:483] Tablets replaced: 457
I0613 12:02:02.931129 52591 master-stress-test.cc:484] Masters restarted: 60

The small difference in number of operations performed is consistent between 
runs, but the relatively small magnitude of the difference leads me to believe 
the timeout isn't necessary.  My theory about why there's not a large effect is 
that the OS should be able to immediately return an IO error to the socket when 
the connection is completely local, at which point the client will immediately 
fail over to another replica.  I haven't checked what happens in cases that the 
client is connecting to a remote master which is killed, but it shouldn't be an 
issue for this itest.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master_hms-itest.cc@139
PS26, Line 139:   Status RenameHmsTable(const std::string& database_name,
> Nit: remove std:: prefix from all instances of std::string.
Done

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-12 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 26:

(12 comments)

Increasing HMS integration test coverage by parameterizing existing tests the 
way you did makes sense, but do any of these tests now need to be sharded? Are 
any approaching the 900s runtime mark, at which point they'd be killed?

http://gerrit.cloudera.org:8080/#/c/8313/26//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8313/26//COMMIT_MSG@16
PS26, Line 16:  As a result, rename and delete RPCs are handled specially to
 : ensure they are applied to the HMS first, and only once they are
 : committed in the HMS are they applied to the Kudu catalog, 
through the
 : new notification log listener.
I would also call out the extra complexity of compound alters that rename 
tables and change schemas. Those generate quite the RPC sequence.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/hms/hms_catalog.cc@440
PS26, Line 440:   // Set the table type to external so that the table's (HD)FS 
directory will
So there's no way to get the HMS to just not create HDFS directories for Kudu 
tables?


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master-stress-test.cc@a160
PS26, Line 160:
I remember a discussion about this on Slack, but I thought you wouldn't need to 
remove it in the end. Won't this dramatically reduce the number of DDLs per 
thread because it'll take much longer for the clients to retry and find the new 
leader when the old one is killed?


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/integration-tests/master_hms-itest.cc@139
PS26, Line 139:   Status RenameHmsTable(const std::string& database_name,
Nit: remove std:: prefix from all instances of std::string.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.h@700
PS26, Line 700:   // Delete the specified table in the catalog.
Also update this one.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.h@994
PS26, Line 994:   static const char* state_to_string(State state);
Nit: wouldn't StateToString be a more style-compliant name?


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.cc@1334
PS26, Line 1334:   // renamed in the HMS lower.
Nit: don't need 'lower'.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.cc@1745
PS26, Line 1745: leader_lock_.AssertAcquiredForReading();
Move this to the top of the function? It's a true assertion regardless of 
whether we're using HMS integration or not.

Oh, I see, you were trying to avoid duplicating it because DeleteTable() will 
do it too. I don't think the duplication is a big deal; it's useful just as a 
comment for anyone reading the code.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.cc@2178
PS26, Line 2178:
Nit: remove std:: prefixes below.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/catalog_manager.cc@3973
PS26, Line 3973:   // It wouldn't typically be correct to write to the field 
under only a read
   :   // lock, however this instance is safe because it's only 
called by the
   :   // HmsNotificationLogListenerTask singleton thread.
Maybe add a note here about how if this assumption is violated (i.e. if someone 
makes a mistake and spawns multiple listener tasks), we'd expect to see TSAN 
warnings.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/hms_notification_log_listener.h
File src/kudu/master/hms_notification_log_listener.h:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/hms_notification_log_listener.h@147
PS26, Line 147:   std::vector catch_up_callbacks_;
Is this really necessary? Seems like you may be able to get by with a second 
condvar, also protected by lock_. Callers of WaitForCatchUp wait on it, and 
RunLoop broadcasts it after every poll iteration.


http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/26/src/kudu/master/hms_notification_log_listener.cc@124
PS26,

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-12 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 26: Code-Review+2

It would be good if Adar reviews it as well.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 26
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 12 Jun 2018 22:43:09 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-12 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
17 files changed, 1,092 insertions(+), 219 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/25
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 25
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-12 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test and alter_table-randomized-test
are now parameterized to run with the HMS integration enabled, which has
been effective at revealing issues.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
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/mini_hms.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
17 files changed, 1,088 insertions(+), 216 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/24
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-07 Thread Hao Hao (Code Review)

Hao Hao has uploaded a new patch set (#20) to the change originally created by 
Dan Burkert. ( http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
14 files changed, 1,037 insertions(+), 207 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/20
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 20
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-05 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/util/test_util.cc
14 files changed, 1,037 insertions(+), 207 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/18
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 18
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-05 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
13 files changed, 1,035 insertions(+), 206 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/17
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 17
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-01 Thread Dan Burkert (Code Review)

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

The HMS integration has known issues with capital letters in table
names. A follow-up commit will introduce a fix and tests for this
issue.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
13 files changed, 1,033 insertions(+), 204 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/16
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 16
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-01 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 16:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc@1549
PS14, Line 1549:   for (const auto& tablet : tablets) {
> Maybe we should call WaitForCatchUp here as well?
Good point, I've added this above, before we check for name uniqueness in the 
Kudu catalog.


http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc@2163
PS14, Line 2163: schema
> nit: alter table event.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 16
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 02 Jun 2018 00:07:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-01 Thread Hao Hao (Code Review)

Hao Hao has uploaded a new patch set (#15) to the change originally created by 
Dan Burkert. ( http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 1,007 insertions(+), 190 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/15
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 15
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-06-01 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc@1549
PS14, Line 1549: Status s = hms_catalog_->CreateTable(table->id(), 
req.name(), schema);
Maybe we should call WaitForCatchUp here as well?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 01 Jun 2018 21:25:38 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

2018-05-31 Thread Hao Hao (Code Review)

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 14:

(3 comments)

Overall, LGTM, but there are some test failures, would you mind checking if 
these are related?

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.cc@1788
PS11, Line 1788: hms_notification_log_event_id_ = notification_log_event_id;
> That's done in the previous call to DeleteTable(..).  It has to be done the
Ah, missed that. Makes sense.


http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/14/src/kudu/master/catalog_manager.cc@2163
PS14, Line 2163: delete
nit: alter table event.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@203
PS11, Line 203:  flag.
  :   int32_t batch_size
> The table rename and ID update should be written to the sys catalog atomica
No, LGTM, thanks for the explanation.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 31 May 2018 23:20:33 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 1,007 insertions(+), 190 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8313/14
--
To view, visit http://gerrit.cloudera.org:8080/8313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 14
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..

KUDU-2191 (12/n): Hive Metastore notification log event listener

This commit adds a notification log event listener to the Hive Metastore
integration, and shifts how the catalog manager handles table metadata
when the HMS integration is enabled. The leader master now listens for
drop table and alter table events in the HMS, and applies them to the
Kudu catalog as necessary. The latest handled notification log event
index is recorded in the sys-catalog. When the HMS integration is
enabled, the HMS is considered the source of truth for the table
namespace. As a result, rename and delete RPCs are handled specially to
ensure they are applied to the HMS first, and only once they are
committed in the HMS are they applied to the Kudu catalog, through the
new notification log listener.

Testing: this is a hard component to test, because it is necessarily
tied to the catalog manager. I've added tests for specific scenarios in
master_hms-itest, however I'm aware that not all codepaths in the
listener are covered. master-stress-test is now parameterized to run
with the HMS integration enabled, since this was the 'missing piece'
which allows the catalogs to remain (eventually) consistent in the
presence of master crashes.

Change-Id: I32ed099c44a593ffe514152135957018f21ed775
---
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
A src/kudu/master/hms_notification_log_listener.cc
A src/kudu/master/hms_notification_log_listener.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
12 files changed, 1,006 insertions(+), 190 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32ed099c44a593ffe514152135957018f21ed775
Gerrit-Change-Number: 8313
Gerrit-PatchSet: 13
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8313 )

Change subject: KUDU-2191 (12/n): Hive Metastore notification log event listener
..


Patch Set 12:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.h@550
PS11, Line 550:   // Delete the specified table in response to a 'DROP TABLE' 
HMS notification
> nit: add a comment.
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.h@562
PS11, Line 562:rpc::RpcContext* rpc);
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.cc@1788
PS11, Line 1788: hms_notification_log_event_id_ = notification_log_event_id;
> Should we store the processed notification log event id in SysCatalogTable
That's done in the previous call to DeleteTable(..).  It has to be done there 
in order to make the write to the SysCatalogTable atomic with the rest of the 
metadata changes.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/catalog_manager.cc@2204
PS11, Line 2204: hms_notification_log_event_id
> It looks like this argument is not used anywhere?
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h
File src/kudu/master/hms_notification_log_listener.h:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h@44
PS11, Line 44: occurrin
> nit: occurring.
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h@84
PS11, Line 84: When invoking this method,
> nit: When invoking this method, the catalog manager must be...
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h@104
PS11, Line 104: Handles an ALTER TABLE event
> Can you elaborate a bit more on what will happen here?
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h@108
PS11, Line 108: tus HandleAlterTableEvent(
> Same here.
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.h@124
PS11, Line 124: // Set to true if the task is
> nit: //  Protected by lock_.
Done


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:

http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@51
PS11, Line 51: runtime
> Do we have to tag it as 'runtime'? In other words, do you expect users to c
I can see this being used as an escape hatch in case an HMS is overloaded, we 
can effectively turn down the load from Kudu by making this a really big 
number.  That being said it's kind of speculative, and maybe no one will ever 
want to do that.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@58
PS11, Line 58: runtime
> Same as above.
Same thing.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@175
PS11, Line 175: : $0", key));
  :   }
> Yeah, I think the best way to handle it is to ignore it as what you are doi
Updated the comment to be more formal.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@203
PS11, Line 203:  flag.
  :   int32_t batch_size
> What if a rename table event is processed but did not get a chance to be pe
The table rename and ID update should be written to the sys catalog atomically, 
so I don't think that can happen.  Or did you have something else in mind?


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@225
PS11, Line 225: led to r
> How do you know this is the message format we should validate against?
I've added some more info about this to the new helper method doc.


http://gerrit.cloudera.org:8080/#/c/8313/11/src/kudu/master/hms_notification_log_listener.cc@309
PS11, Line 309: if (before_table_name == after_table_name) {
  : VLOG(2) << "Ignoring non-rename alter table event on table "
  : << *table_id << " " << before_table_name;
  : return Status::OK();
  :   }
  :
  :   RETURN_NOT_OK(catalog_manager_->RenameTableHms(*table_id,
  :  
before_table_name,
  :  
after_table_name,
  :  
event.eventId));
  :   *durable_event_id = event.eventId;
  :   return

[kudu-CR] KUDU-2191 (12/n): Hive Metastore notification log event listener