You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2018/06/01 23:37:57 UTC

[kudu-CR] KUDU-2191: Add 'kudu only' flag 

Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10581


Change subject: KUDU-2191: Add 'kudu_only' flag
......................................................................

KUDU-2191: Add 'kudu_only' flag

This commit adds a 'kudu_only' flag to CreateTable/DropTable/AlterTable
APIs to indicate whether to the table creation/deletion/altering should
take place in the Hive Metastore or not. The default value for the flag
is false, which means the operation will take effect in the Hive
Metastore.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 98 insertions(+), 36 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................

KUDU-2191: Add a method to decide the scope of AlterTable

This commit adds a private method in KuduTableAlterer to indicate
whether the table alteration should be applied to external catalogs,
such as the Hive Metastore, which the Kudu master may has been
configured to integrate with.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Reviewed-on: http://gerrit.cloudera.org:8080/10581
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 41 insertions(+), 8 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Hao Hao: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2191: Add 'kudu only' flag

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

Change subject: KUDU-2191: Add 'kudu_only' flag
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h@339
PS1, Line 339:   Status DeleteTable(const std::string& table_name, bool kudu_only = false);
> warning: function 'kudu::client::KuduClient::DeleteTable' has a definition 
I don't think adding a new parameter (even with a default) is ABI-compatible (here and below). Adar's the expert on this though



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Jun 2018 18:02:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag

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

Change subject: KUDU-2191: Add 'kudu_only' flag
......................................................................


Patch Set 1:

(1 comment)

> I don't think this flag should be exposed in public APIs because
 > it's not something many users will care about. What's the argument
 > for making it public?

Makes sense, the only users for this change would be the HMS tools as what I can tell for now. So making it private instead.

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h@347
PS1, Line 347:   KuduTableAlterer* NewTableAlterer(const std::string& table_name);
> warning: function 'kudu::client::KuduClient::NewTableAlterer' has a definit
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 07 Jun 2018 07:21:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 4:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@45
PS4, Line 45: #include "kudu/master/master.pb.h"
This can't be included here; it's not part of the public client headers (and shouldn't be, since it's an internal header).


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@1194
PS4, Line 1194:   KuduTableAlterer* system(master::AlterTableRequestPB::SystemType system);
You can't use a PB-defined enum here. That's why my original suggestion differentiated between an enum (used in the wire protocol, where we must maintain ABI compatibility) and a bool (used here in the private API, which we can change at will).


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h@82
PS4, Line 82:       master::AlterTableRequestPB::NO_HMS;
Shouldn't the default be ALL_SYSTEMS?


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

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@48
PS4, Line 48: // Note: this test needs to be in the client namespace in order for
You don't need to do this. See how ClientStressTest is used in FRIEND_TEST in KuduClient for an example.


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@360
PS4, Line 360:   bool exist;
Nit: exists


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

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/catalog_manager.cc@2158
PS4, Line 2158:       req.system() == master::AlterTableRequestPB::ALL_SYSTEMS) {
To make this more future-proof, change the condition to != NO_HMS. That way if we add a third entry that disables some other external connector, this still works.


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@570
PS4, Line 570:   enum SystemType {
Ni:t how about ExternalSystemType:?


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@571
PS4, Line 571:     ALL_SYSTEMS = 0;
Nit: just ALL is enough, given that to use the enum value you need to qualify it with SystemType anyway.


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@574
PS4, Line 574:   optional SystemType system = 5 [ default = ALL_SYSTEMS ];
Nit: how about system_to_alter.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 08 Jun 2018 03:39:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto@568
PS3, Line 568:   // Whether to alter the table in Kudu as well as in the Hive Metastore,
> Done
Sorry I'm late to this, but I don't agree with this change.  'NO_HMS' isn't a system type, it's the lack of a type.  'ExternalSystem' is too general, as well.  I'd prefer

    // Whether to apply the alteration to external catalogs, such as the Hive Metastore,
    // which the Kudu master has been configured to integrate with.
    optional bool alter_external_catalogs = 5 [default = true];

As far as enum vs bool, I don't think the enum is any more future proof than a boolean.  It's impossible to know if/when we integrate with another external catalog system whether the enum will be useful vs just a boolean.  I do see value in making it slightly less HMS specific, though.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 21:11:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 23:21:15 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto@568
PS3, Line 568:   // Whether to alter the table in Kudu as well as in the Hive Metastore,
> Sorry I'm late to this, but I don't agree with this change.  'NO_HMS' isn't
I'm OK with Dan's suggestion too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 21:20:31 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/client/client.h@1193
PS3, Line 1193:   // method returns a raw pointer to this alterer object.
> How about modeling this as a private KuduTableAlterer mutator? Like this:
Done


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

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/integration-tests/master_hms-itest.cc@453
PS3, Line 453:   ASSERT_TRUE(s.IsNotFound()) << s.ToString();
> Dan mentioned in the past that tests which start up an HMS are super slow. 
Done


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

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/catalog_manager.cc@2158
PS3, Line 2158:       req.system() == master::AlterTableRequestPB::ALL_SYSTEMS) {
> The default value of a bool in PB is false, so you don't need to check has_
Done


http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto@568
PS3, Line 568:   // Whether to alter the table in Kudu as well as in the Hive Metastore,
> How about defining this as an enum that describes which external systems sh
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 08 Jun 2018 00:46:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................

KUDU-2191: Add a method to decide the scope of AlterTable

This commit adds a private method in KuduTableAlterer to indicate
whether the table alteration should take place in the Hive Metastore or
not when HMS integration is enabled.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 48 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................

KUDU-2191: Add a method to decide the scope of AlterTable

This commit adds a private method in KuduTableAlterer to indicate
whether the table alteration should be applied to external catalogs,
such as the Hive Metastore, which the Kudu master may has been
configured to integrate with.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 41 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10581/5/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/10581/5/src/kudu/client/client.cc@1144
PS5, Line 1144:     bool alter_external_catalogs) {
> This doesn't do the right thing if you call system(true) followed by system
Done


http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto@568
PS3, Line 568:   // Whether to apply the alteration to external catalogs, such as the Hive Metastore,
> Sorry I'm late to this, but I don't agree with this change.  'NO_HMS' isn't
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 22:54:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag to AlterTable

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

Change subject: KUDU-2191: Add 'kudu_only' flag to AlterTable
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h@339
PS1, Line 339:   /// @param [in] table_name
> KDE wiki's page on ABI compatibility [1] (linked from client/CMakeLists.txt
Thanks for pointing this out, updated the method to be private.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 07 Jun 2018 07:24:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 8: Code-Review+2

Carry over +2 from Adar and Dan.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 15 Jun 2018 18:18:36 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 23:29:54 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 5:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@45
PS4, Line 45: #include "kudu/util/kudu_export.h"
> This can't be included here; it's not part of the public client headers (an
Done


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/client.h@1194
PS4, Line 1194:   KuduTableAlterer* system(bool kudu_only);
> You can't use a PB-defined enum here. That's why my original suggestion dif
Done


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/client/table_alterer-internal.h@82
PS4, Line 82:       master::AlterTableRequestPB::ALL_SYSTEMS;
> Shouldn't the default be ALL_SYSTEMS?
Oops, my bad.


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

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@48
PS4, Line 48: using client::KuduColumnSchema;
> You don't need to do this. See how ClientStressTest is used in FRIEND_TEST 
Done


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/integration-tests/master_hms-itest.cc@360
PS4, Line 360:   ASSERT_OK(hms_client_->GetTable("default", "a", &hms_table));
> Nit: exists
Done


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

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/catalog_manager.cc@2158
PS4, Line 2158:       req.system_to_alter() != master::AlterTableRequestPB::NO_HMS) {
> To make this more future-proof, change the condition to != NO_HMS. That way
Done


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@570
PS4, Line 570:   enum ExternalSystemType {
> Ni:t how about ExternalSystemType:?
Done


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@571
PS4, Line 571:     ALL_SYSTEMS = 0;
> Nit: just ALL is enough, given that to use the enum value you need to quali
Hmm, I used ALL_SYSTEMS because it seems SystemType is not needed to refer the enum value. In the proto buff auto-generated code it has 'static const SystemType ALL_SYSTEMS =
    AlterTableRequestPB_SystemType_ALL_SYSTEMS;'


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@574
PS4, Line 574:   optional ExternalSystemType system_to_alter = 5 [ default = ALL_SYSTEMS ];
> Nit: how about system_to_alter.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 19:42:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag

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

Change subject: KUDU-2191: Add 'kudu_only' flag
......................................................................


Patch Set 1:

I don't think this flag should be exposed in public APIs because it's not something many users will care about. What's the argument for making it public?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sun, 03 Jun 2018 20:27:50 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2191: Add 'kudu only' flag to AlterTable

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

Change subject: KUDU-2191: Add 'kudu_only' flag to AlterTable
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/client/client.h@1193
PS3, Line 1193:   Status AlterTable(bool kudu_only);
How about modeling this as a private KuduTableAlterer mutator? Like this:

  // Whether to alter the table in Kudu as well as in any connected external metadata system, or just in Kudu itself.
  KuduTableAlterer* kudu_only(bool kudu_only);

Then Alter() remains the only way to perform the alter table.


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

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/integration-tests/master_hms-itest.cc@453
PS3, Line 453: TEST_F(MasterHmsTest, TestKuduOnly) {
Dan mentioned in the past that tests which start up an HMS are super slow. Maybe you could incorporate this into TestAlterTable?


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

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/catalog_manager.cc@2158
PS3, Line 2158:       (!req.has_kudu_only() || !req.kudu_only())) {
The default value of a bool in PB is false, so you don't need to check has_kudu_only.


http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/3/src/kudu/master/master.proto@568
PS3, Line 568:   // Whether to alter the corresponding table in the Hive Metastore or not.
How about defining this as an enum that describes which external systems should be affected by the alter? Then you could have entries like ALL, NO_HMS, etc. It'll be more descriptive that way.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 07 Jun 2018 19:45:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag

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

Change subject: KUDU-2191: Add 'kudu_only' flag
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/10581/1/src/kudu/client/client.h@339
PS1, Line 339:   Status DeleteTable(const std::string& table_name, bool kudu_only = false);
> I don't think adding a new parameter (even with a default) is ABI-compatibl
KDE wiki's page on ABI compatibility [1] (linked from client/CMakeLists.txt) remains my go to resource. In this case "You cannot...For existing functions of any type: change its signature. This includes: extending a function with another parameter, even if this parameter has a default argument." Here's the suggested workaround.

  If you need to add extend/modify the parameter list of an existing function, you need to add a new function instead with the new parameters. In that case, you may want to add a short note that the two functions shall be merged with a default argument in later versions of the library:

    void functionname( int a );
    void functionname( int a, int b ); //BCI: merge with int b = 0

1. https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C++https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 05 Jun 2018 18:11:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag to AlterTable

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2191: Add 'kudu_only' flag to AlterTable
......................................................................

KUDU-2191: Add 'kudu_only' flag to AlterTable

This commit adds a private method in KuduTableAlterer with a 'kudu_only'
flag to indicate whether the table altering operation should take place
in the Hive Metastore or not when HMS integration is enabled.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
5 files changed, 44 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................

KUDU-2191: Add a method to decide the scope of AlterTable

This commit adds a private method in KuduTableAlterer to indicate
whether the table alteration should take place in the Hive Metastore or
not when HMS integration is enabled.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 52 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2191: Add a method to decide the scope of AlterTable

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

Change subject: KUDU-2191: Add a method to decide the scope of AlterTable
......................................................................


Patch Set 5:

(2 comments)

Would like Dan to review this too.

http://gerrit.cloudera.org:8080/#/c/10581/5/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/10581/5/src/kudu/client/client.cc@1144
PS5, Line 1144:   if (kudu_only) {
This doesn't do the right thing if you call system(true) followed by system(false).

It's certainly contrived, but it's easy to make it work properly, so we may as well.


http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/10581/4/src/kudu/master/master.proto@571
PS4, Line 571:     ALL_SYSTEMS = 0;
> Hmm, I used ALL_SYSTEMS because it seems SystemType is not needed to refer 
I see. I thought C++11 protoc generated enum classes. Guess not.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 11 Jun 2018 19:56:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191: Add 'kudu only' flag to AlterTable

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

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

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

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

Change subject: KUDU-2191: Add 'kudu_only' flag to AlterTable
......................................................................

KUDU-2191: Add 'kudu_only' flag to AlterTable

This commit adds a private method in KuduTableAlterer with a 'kudu_only'
flag to indicate whether the table altering operation should take place
in the Hive Metastore or not when HMS integration is enabled.

This method will be useful to the HMS integration tools, such as the
metadata upgrade tool which may need to alter tables in Kudu only.

Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
5 files changed, 42 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I703f12b95bb7ca2d65455f0f0602520332b3c678
Gerrit-Change-Number: 10581
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>