You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/09/21 22:34:20 UTC

[kudu-CR] Add thrift module for common thrift utilities 

Hello Adar Dembo, Hao Hao,

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

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

to review the following change.


Change subject: Add thrift module for common thrift utilities
......................................................................

Add thrift module for common thrift utilities

The HMS patch series inlined all necessary Thrift utilities into the hms
module, since it was the only use of Thrift in the codebase. Now that
we're also planning on having a Sentry client it makes sense to properly
abstract the common Thrift code into its own shared module.

Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
---
M CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
A src/kudu/thrift/CMakeLists.txt
A src/kudu/thrift/client.cc
A src/kudu/thrift/client.h
R src/kudu/thrift/sasl_client_transport.cc
R src/kudu/thrift/sasl_client_transport.h
M src/kudu/tools/kudu-tool-test.cc
16 files changed, 233 insertions(+), 100 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>

[kudu-CR] Add thrift module for common thrift utilities

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

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

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

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

Change subject: Add thrift module for common thrift utilities
......................................................................

Add thrift module for common thrift utilities

The HMS patch series inlined all necessary Thrift utilities into the hms
module, since it was the only use of Thrift in the codebase. Now that
we're also planning on having a Sentry client it makes sense to properly
abstract the common Thrift code into its own shared module.

Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
---
M CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
A src/kudu/thrift/CMakeLists.txt
A src/kudu/thrift/client.cc
A src/kudu/thrift/client.h
R src/kudu/thrift/sasl_client_transport.cc
R src/kudu/thrift/sasl_client_transport.h
M src/kudu/tools/kudu-tool-test.cc
16 files changed, 232 insertions(+), 104 deletions(-)


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

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

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
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-Comment-Date: Fri, 21 Sep 2018 23:27:18 +0000
Gerrit-HasComments: No

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
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-Comment-Date: Sat, 22 Sep 2018 00:30:14 +0000
Gerrit-HasComments: No

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................


Patch Set 2:

As a reminder, this thrift code doesn't contain any unit tests because it can only be exercised by way of connecting to an external server (since we don't have the necessary code to run a Thrift server ourselves).  So, it's well exercised by the existing HMS tests.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
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-Comment-Date: Fri, 21 Sep 2018 23:03:16 +0000
Gerrit-HasComments: No

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11493/2/src/kudu/hms/hms_client.h@93
PS2, Line 93:   // Create an HmsClient connection to the proided HMS Thrift RPC address.
Nit: provided (since you're already in the area)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
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-Comment-Date: Fri, 21 Sep 2018 23:06:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11493/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/11493/1/src/kudu/hms/hms_client.h@97
PS1, Line 97:   // Starts the HMS client.
> warning: function 'kudu::hms::HmsClient::HmsClient' has a definition with d
Done


http://gerrit.cloudera.org:8080/#/c/11493/1/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:

http://gerrit.cloudera.org:8080/#/c/11493/1/src/kudu/hms/hms_client.cc@51
PS1, Line 51: using kudu::thrift::SaslException;
> warning: using decl 'SaslClientTransport' is unused [misc-unused-using-decl
Done


http://gerrit.cloudera.org:8080/#/c/11493/1/src/kudu/hms/hms_client.cc@53
PS1, Line 53: using std::string;
> warning: using decl 'make_shared' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
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-Comment-Date: Fri, 21 Sep 2018 23:00:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add thrift module for common thrift utilities

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

Change subject: Add thrift module for common thrift utilities
......................................................................

Add thrift module for common thrift utilities

The HMS patch series inlined all necessary Thrift utilities into the hms
module, since it was the only use of Thrift in the codebase. Now that
we're also planning on having a Sentry client it makes sense to properly
abstract the common Thrift code into its own shared module.

Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Reviewed-on: http://gerrit.cloudera.org:8080/11493
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
A src/kudu/thrift/CMakeLists.txt
A src/kudu/thrift/client.cc
A src/kudu/thrift/client.h
R src/kudu/thrift/sasl_client_transport.cc
R src/kudu/thrift/sasl_client_transport.h
M src/kudu/tools/kudu-tool-test.cc
16 files changed, 233 insertions(+), 105 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
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

[kudu-CR] Add thrift module for common thrift utilities

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

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

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

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

Change subject: Add thrift module for common thrift utilities
......................................................................

Add thrift module for common thrift utilities

The HMS patch series inlined all necessary Thrift utilities into the hms
module, since it was the only use of Thrift in the codebase. Now that
we're also planning on having a Sentry client it makes sense to properly
abstract the common Thrift code into its own shared module.

Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
---
M CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/hms/hms_catalog.cc
M src/kudu/hms/hms_catalog.h
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
A src/kudu/thrift/CMakeLists.txt
A src/kudu/thrift/client.cc
A src/kudu/thrift/client.h
R src/kudu/thrift/sasl_client_transport.cc
R src/kudu/thrift/sasl_client_transport.h
M src/kudu/tools/kudu-tool-test.cc
16 files changed, 233 insertions(+), 105 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f6f843f42b37cb1170df03da01fc0790fe94acb
Gerrit-Change-Number: 11493
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
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