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 2017/06/02 01:35:50 UTC
[kudu-CR] WIP: Add basic Hive MetaStore client
Dan Burkert has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7053
Change subject: WIP: Add basic Hive MetaStore client
......................................................................
WIP: Add basic Hive MetaStore client
This is a work-in-progress patch to create a basic HiveMetastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependency.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
WIP because it's not yet expected to work with TSAN (thrift needs to be
built in TSAN mode), or dist-test (Hive and Hadoop artifacts need to be
distributed).
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
15 files changed, 2,127 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/1
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#19).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 3,047 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/19
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:45:40 +0000
Gerrit-HasComments: No
[kudu-CR] Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#20).
Change subject: Hive MetaStore client
......................................................................
Hive MetaStore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,851 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/20
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 20
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#13).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 2,999 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/13
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> That's a lot. By the way of comparison, the unified LLVM tarball (which I b
ok I stripped it. You're going to be very dissapointed with the hadoop/hive tarballs. Even after stripping the docs they are about ~320MiB combined
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#11).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,619 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/11
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#14).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 2,999 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/14
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#23).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,917 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/23
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 23
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 22:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@31
PS22, Line 31: kudu_common
> Not done? I still see kudu_common in HMS_DEPS.
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc@23
PS5, Line 23:
> I want to punt on this until we have two cases where it's necessary, I'm st
OK so this has come back to haunt me in a big way.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc@125
PS22, Line 125: static const string kFileTemplate = R"(
> Could you also add a comment for hive.metastore.event.db.listener.timetoliv
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@730
PS22, Line 730: rm -Rf *
> OK, but every single rebuild of thirdparty is going to have to rebuild thri
OK I'm going to remove this until the issue reappears. I don't see the point in knowing how long it takes to build a cold Thrift when the alternative is that it doesn't build at all.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: HIVE_VERSION=3fb4649fa847cfec33f701f6c884f12087680cf0
> OK but could you at least leave a breadcrumb here pointing to the git repos
I'm not sure, I always just download the tarball via GitHub's 'download' button.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 22
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:28:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed a vote on this change.
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 7:
(69 comments)
http://gerrit.cloudera.org:8080/#/c/7053/6//COMMIT_MSG
Commit Message:
Line 9: This patch lays the groundwork for a Hive metastore client.
> any progress on the design doc for this work? Would be good to be able to r
Not yet, but I plan to delay landing this until the design work is more concrete, so I'll make sure to add a link then.
http://gerrit.cloudera.org:8080/#/c/7053/6/CMakeLists.txt
File CMakeLists.txt:
Line 829: # We rely on the JVM for running the HMS during tests.
> do we need to update any build docs to specify that java is now a requireme
I'm not sure. I've changed this so that these dependencies are only required if !NO_TESTS. So our options are to add Java as a dependency when building from source, or to add NO_TESTS to the from-source build instructions. Not sure which I prefer, I'll try and take an informal poll.
http://gerrit.cloudera.org:8080/#/c/7053/5/build-support/dist_test.py
File build-support/dist_test.py:
Line 82: # Hive MetaStore tests require hadoop and hive libraries. These files are
> These directories will be recursively copied? Or are these just symlinks? C
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/build-support/run_dist_test.py
File build-support/run_dist_test.py:
Line 127: # are used in mini_hms.cc.
> Can you use ROOT to derive the paths instead of relying on the cwd?
Done
Line 129: env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0]
> Can we run detect-java-home.sh from bigtop instead? This seems rather fragi
This is how other dist-test projects do it: https://github.com/cloudera/dist_test/blob/master/grind/python/disttest/isolate.py#L114
http://gerrit.cloudera.org:8080/#/c/7053/5/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
Line 19: # https://github.com/apache/bigtop/blob/38e1571b2f73bbfa6ab0c01a689fae967b8399d9/bigtop-packages/src/common/bigtop-utils/bigtop-detect-javahome
> If this is a specific version for a reason, please explain why. IIRC, it's
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
Line 64: set(JAVA_HOME $ENV{JAVA_HOME})
> can we do something here to check that EXISTS ${JAVA_HOME}/bin/java just in
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/cmake_modules/FindThrift.cmake
File cmake_modules/FindThrift.cmake:
PS5, Line 106: list(APPEND ${SRCS} "${THRIFT_CC_OUT}" "fb303_types.cpp" "fb303_constants.cpp" "FacebookService.cpp")
: list(APPEND ${HDRS} "${THRIFT_H_OUT}" "
> Remove this?
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/cmake_modules/FindThrift.cmake
File cmake_modules/FindThrift.cmake:
PS6, Line 105: # TODO(dan): Add the fb303 files manually. This is a complete hack.
: list(APPEND ${SRCS} "${THRIFT_CC_OUT}" "fb303_types.cpp" "fb303_constants.cpp" "FacebookService.cpp")
: list(APPEND ${HDRS} "${THRIFT_H_OUT}" "fb303_types.h" "fb303_constants.h" "FacebookService.h")
:
> hrm, could we at least add this as an optional argument like REQUIRE_FB303?
Yah perhaps. Right now we only have one consumer, so I'd like to punt on this (I already find this script complex).
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
PS5, Line 21:
: add_libra
> This is only used in one place, just inline it there?
Done
Line 23: target_link_libraries(hms_thrift thrift)
> Is hms_thrift going to be linked into the Kudu client library? If not, it d
Done
Line 35:
> Likewise.
Done
Line 44: execute_process(COMMAND ln -sf
> Nit: use lowercase for cmake built-ins (i.e. add_custom_target(...)).
Done
Line 49: "${EXECUTABLE_OUTPUT_PATH}/java-home")
> So this means the symlinks will be recreated on every invocation of "make"
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
Line 23: target_link_libraries(hms_thrift thrift)
> since the client won't use hms at all, I think we can use the built-in ADD_
Done
Line 35:
> same
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 23
> But we're not explicitly including boost here. Is this from ThriftHiveMetas
Then it would be leaking into non-test code. It's also harder because it's a generated header.
Line 42
> Also test that Stop() works?
Done
Line 52
> Surprised the compiler needs the vector<string> part and isn't able to figu
It doesn't compile without it, not sure why.
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 53
> I think if you include stl_logging.h then you don't need to do this << part
Done
Line 61
> is there a gaurantee that they come back in sorted order or is a std::sort(
Done
Line 62
> same
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore.cc
File src/kudu/hms/hive_metastore.cc:
Line 49
> Are these all of the exception classes that may be thrown? Is there some wa
TException is something of a catch-all case, although it's not unexepected (IO errors fall under TException). My understanding is that will catch anything thrown from thrift.
Line 63
> So client_ is actually a pointer? Is the pointer hidden behind a typedef or
No I think this is just a ctor argument (boost::shared_ptr<TProtocol>).
Line 71
> Maybe WARN_NOT_OK() instead? I think that's the convention we use.
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore.cc
File src/kudu/hms/hive_metastore.cc:
PS6, Line 53:
> compared to the other macros we have like RETURN_NOT_OK_PREPEND, it's odd t
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore.h
File src/kudu/hms/hive_metastore.h:
Line 33
> Maybe HiveMetastoreClient then? Or HMSClient? HiveMetastore suggests the ac
Done
Line 39
> What happens if Start or Stop are called multiple times, or if Stop is call
They can be called multiple times, but the bound port will change. I don't think it's worth worrying about - we can cross that bridge if we need fault tests.
Line 51
> Forward declare and maybe avoid some includes?
Done
Line 57
> Guess we can't forward declare this, so maybe forward declaring the above i
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore.h
File src/kudu/hms/hive_metastore.h:
PS6, Line 51:
> I think GSG discourages namespace aliases, but maybe we should make an exce
SGTM.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms-test.cc
File src/kudu/hms/mini_hms-test.cc:
Line 24
> MiniHmsTest
Done
Line 26
> Shouldn't we also test some sort of RPC?
I'm just going to get rid of this test since it's duplicated by the hms_client-test entirely. I originally wrote it because I created mini_hms before hms_client.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 20: #include <csignal>
> Don't need?
It's needed for SIGKILL.
Line 27: #include <glog/logging.h>
> Don't need?
Done
Line 28:
> Don't need?
Done
Line 33: #include "kudu/util/status.h"
> Don't need?
needed for SCOPED_LOG_SLOW_EXECUTION
Line 47: VLOG(1) << "Stopping HMS";
> WARN_NOT_OK() here?
Done
Line 60: const char* env = std::getenv(env_var.c_str());
> Use a ternary?
Done
Line 63: if (!Env::Default()->FileExists(*home_dir)) {
> Extra space here.
Done
Line 65: }
> Shouldn't we gracefully return this sort of error via Status?
First part is done. Second part - I don't really think that's worth the trouble, hive should complain about it appropriately.
Line 72: SCOPED_LOG_SLOW_EXECUTION(WARNING, 20000, "Starting HMS");
> Remove? Or uncomment?
Done
Line 77: Env* env = Env::Default();
> How about asking users to supply this in the class constructor?
Why? This isn't how mini_kdc works.
Line 106: schematool.SetCurrentDir(tmp_dir);
> Nit: do you actually need to initialize the value? Would prefer to adhere t
I see you like to tempt the UB demons.
Line 109: int rc;
> Maybe include the exit status? Should use GetExitStatus() anyway.
I'm skeptical that anyone can glean information from the exit status of schemaTool.
Line 112: return Status::RuntimeError("schematool failed");
> Kerberos HMS?
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 65: }
> nit: extra space
Done
Line 91: auto db_path = JoinPathSegments(tmp_dir, "metastore_db");
> vlog?
Done
PS6, Line 114:
> copy pasta
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/security/test/mini_kdc.cc
File src/kudu/security/test/mini_kdc.cc:
Line 151: RETURN_NOT_OK(WaitForUdpBind(kdc_process_->pid(), &options_.port, MonoDelta::FromSeconds(1)));
> This seems like a rather short timeout. The old code was even more aggressi
It hasn't seemed to be a problem. The kdc should be fast.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 749
> Nit: remove unnecessary this prefixes, here and below.
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
PS6, Line 760:
:
> this was a bit specific to the KDC case, I think could be cleaned up a bit.
I don't think it's specific to the KDC, it's going to take some time for any process to start up. The HMS, for instance, takes on the order of 5 seconds.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
Line 205
> "to the specified executable"
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
PS6, Line 160: t { return argv_[0]; }
> "bind to ANY listening..."?
Done
PS6, Line 205:
> typo
Done
PS6, Line 206:
> you mean the $PATH environment variable?
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
Line 620: Source: https://thrift.apache.org
> Copy/pasta
Done
Line 658: Source: https://www.gnu.org/software/bison
> Copy/pasta?
Done
http://gerrit.cloudera.org:8080/#/c/7053/6/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
Line 620: Source: https://thrift.apache.org
> wrong paste?
Done
Line 658: Source: https://www.gnu.org/software/bison
> same
Done
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 501: CPPFLAGS="$EXTRA_CPPFLAGS $OPENSSL_CFLAGS" \
> Isn't this wrong though? Curl is a C program so I'd expect CXXFLAGS has no
Done
Line 671: ./bootstrap.sh --prefix=$PREFIX threading=multi --with-libraries=date_time
> Is this actually needed? I don't see a corresponding change to FindKuduBoos
Done
PS5, Line 698:
> Why do we need these two additions? I can take my answer in the form of a c
Done
Line 732: LDFLAGS="$EXTRA_LDFLAGS" \
> Should be cp -f, right? Otherwise will it overwrite?
We don't use -f anywhere else in this file. Could you elaborate on why it's needed?
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 323: fi
> No autoreconf?
autoreconf is failing on OS X.
Line 331: fi
> No autoreconf?
Yah, it breaks on OS X, and this is a build-only dependency.
Line 342
> Perhaps we should follow our LLVM tarball's lead and remove it a priori, to
We don't do that for LLVM. https://github.com/apache/kudu/blob/master/thirdparty/package-llvm.sh
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/vars.sh
File thirdparty/vars.sh:
Line 180: BISON_VERSION=3.0.4
> Why use this version of bison? The latest is 3.0.4 and it's quite old alrea
Done
Line 185: HIVE_NAME=apache-hive-$HIVE_VERSION-bin
> Why not just hive-$HIVE_VERSION?
Because they name their tarball contents in different way.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> Yeah, IWYU's recommendations for this file don't make sense. Could you let
I'll take a look at that.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:10:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#37).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/37
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 37
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 714: -DWITH_OPENSSL=OFF \
> Why "may need to"? Now that we're encrypting server-to-server communication
Sure, but that doesn't necessarily mean that OpenSSL is required, and in fact I think it's pretty unlikely that the HMS uses TLS in any capacity.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> unique_ptr doesn't work with forward declared classes.
It depends on what 'work' is required :)
The funny fact here is that if you move the definition of the MiniHms constructor into the .cc file, it will work with forward-declared Subprocess class.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:31:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 26:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7053/25/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/25/src/kudu/hms/hms_client.cc@99
PS25, Line 99: shared_ptr<TSocket> socket(new TSocket(hms_address.host(), hms_address.port()));
Can we use std::make_shared here?
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@125
PS24, Line 125: 'datanucleus.schem
> Which options is this referring to? datanucleus.schema.autoCreateAll and hi
I think you missed this one.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: BISON_NAME=bison-$BISON_VERSION
> I'm not sure, I always just download the tarball via GitHub's 'download' bu
PS26 has more info about how we don't use a release, but the provenance is still incomplete (i.e. it needs a URL to a repository).
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh@203
PS26, Line 203: # TODO(dan): bump to 0.11 when it's released. We chose to use a bleeding edge
Could you also include a URL here to the Thrift repo?
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh@206
PS26, Line 206: .
Nit: got an extra period here.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 26
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:45:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.cc@121
PS29, Line 121: return WaitForTcpBind(hms_process_->pid(), &port_, MonoDelta::FromSeconds(20));
> Any particular reason this timeout shouldn't be ridiculously high? Like, 90
No tests wait for this to elapse unless Hive fails to start up. I verified 20 seconds is long enough on a dist-test run.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 23:04:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#24).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,914 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/24
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 36:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30
PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep
> Any particular reason you don't want to move MiniHms constructor into mini_
I don't like the idea of making the code more complex in order to satisfy IWYU.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:41:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 23:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt@870
PS22, Line 870: find_package(Java 1.7 REQUIRED)
> Could you add a comment that the version argument is >=, not == ?
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt@871
PS22, Line 871: # Defines the add_jar() CMake command.
> Can you add a comment saying that this allows us to call add_jar()?
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
http://gerrit.cloudera.org:8080/#/c/7053/22/cmake_modules/FindJavaHome.cmake@74
PS22, Line 74: set(JAVA_HOME_FOUND true)
> Nit: I think it'd be clearer if this were inverted. That is, if this were u
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@31
PS22, Line 31: kudu_common
> Why is this needed?
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@57
PS22, Line 57: INCLUDE_JARS ${DEPENDENCY_JARS}
From the CMake docs:
> We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@59
PS22, Line 59:
> What does EXECUTABLE_OUTPUT_PATH resolve to? Where does the JAR end up?
in build/latest/bin/
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@64
PS22, Line 64: target_link_libraries(mini_hms
> If this is to be used by the minicluster CLI tool, it can't be conditioned
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@73
PS22, Line 73: kudu_hms
> FWIW, 'hms' and 'mini-hms' side-by-side suggest that both are metastore imp
renamed to 'kudu_hms' to match the rest of our namespace -> lib name mappings.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/hive_metastore.thrift@22
PS22, Line 22: # https://raw.githubusercontent.com/apache/hive/rel/release-2.3.0/metastore/if/hive_metastore.thrift
> Could you add a note in vars.sh next to Hive that if we bump the version, w
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc@125
PS22, Line 125: // The schema options allow Hive to startup and run without first running the
> Would be nice to sprinkle comments here explaining the choice for each non-
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
PS22:
> What about hadoop and hive?
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@726
PS22, Line 726: build_thrift() {
> Should mention that this depends on bison. See build_glog for inspiration.
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@730
PS22, Line 730: # Normally we would only remove cached CMake files, but the Thrift build is
> Why is this necessary? None of our other dependency builds do this.
Comment added. This was changed in R17 as a result of failed builds on CI. The logs are gone and I don't recall exactly what the symptoms were.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@737
PS22, Line 737: # Thrift requires C++11 when compiled on Linux against libc++ (see cxxfunctional.h).
> Add EXTRA_CMAKE_FLAGS and then use ${NINJA:-make} instead of make.
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh@343
PS22, Line 343: THRIFT_PATCHLEVEL=0
> Apparently we should now be setting PATCHLEVEL=0 on new deps to ease future
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh@349
PS22, Line 349: fi
:
> Can you add a little more detail? Do you remember which platform failed?
Done
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: # TODO(dan): bump to a release version once HIVE-17747 is published.
> How was this built? Is it sourced from github? Could you include instructio
Yes it was sourced from the apache hive git repository. We should not be doing this in the future, it should just be a Hive release tarball once the next Hive release is cut. We can't do our normal patch thing here because we're consuming binary tarballs.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 23
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 00:42:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 27:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@101
PS24, Line 101: map<string, string> env_vars {
> nit: const ?
Why should this variable in particular be const?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: BISON_NAME=bison-$BISON_VERSION
> PS26 has more info about how we don't use a release, but the provenance is
It's a SHA from the upstream Apache git repository. Can we not assume contributors to Apache Kudu can find the Apache Hive repository?
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh@203
PS26, Line 203: # TODO(dan): bump to 0.11 when it's released. We chose to use a bleeding edge
> Could you also include a URL here to the Thrift repo?
The Apache project page is linked to in the license file. The apache git repo has a pretty useless web ui.
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh@206
PS26, Line 206: .
> Nit: got an extra period here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:26:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc@25
PS29, Line 25: #include <glog/logging.h>
: #include <glog/stl_logging.h>
I think IWYU is right about removing these; I don't see any LOG/VLOG usage here.
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include <vector>
Maybe IWYU wants to remove this because:
1. All of the references are OUT by-pointer params, and
2. One of the included headers forward declares std::vector?
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
Yeah, IWYU's recommendations for this file don't make sense. Could you let Alexey know?
The problem with ignoring IWYU outright is that the next patch to touch this file will continue to fail IWYU.
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
IWYU's recommendation to remove this makes sense: you can forward declare Subprocess.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 23:20:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#25).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,911 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/25
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 25
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 36:
OK I think this is ready to review again. Sorry for the churn.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 18:40:18 +0000
Gerrit-HasComments: No
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#9).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
21 files changed, 2,474 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/9
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(14 comments)
I've added TODOs to expand the error handling behavior and documentation in HmsClient. I'd like for these things to be tested thoroughly, but this commit is already quite large when considering the build changes, so I'd like to leave it to a follow-up commit.
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128: env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/apache-hive-*-bin"))[0]
: env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0]
> when we end up revving these dependencies it seems likely we'll end up with
Done
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt@40
PS30, Line 40: execute_process(COMMAND ln -nsf
: "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hive"
: "${EXECUTABLE_OUTPUT_PATH}/hive-home")
: execute_process(COMMAND ln -nsf
: "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop"
: "${EXECUTABLE_OUTPUT_PATH}/hadoop-home")
: execute_process(COMMAND ln -nsf
: "${JAVA_HOME}"
: "${EXECUTABLE_OUTPUT_PATH}/java-home")
> does it make more sense to do these in build-thirdparty? even though it's n
I don't think it's possible, since we can't know what directory the user will build Kudu in during the thirdparty build.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift@21
PS30, Line 21: # DO NOT MODIFY! Copied from
> should we be setting any C++ specific options like 'moveable types'?
I added moveable_types to FindThrift generation. I don't think it really buys us much since all the service interfaces still takes args by const ref, but I guess it helps for building the arguments to begin with.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc@103
PS30, Line 103: ASSERT_TRUE(CreateTable(&client, database_name, table_name, "").IsRuntimeError());
> i think it's worth ASSERT_STR_MATCHES the error string here since RuntimeEr
Done
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@60
PS30, Line 60: // can not be reached, an error is returned.
> is there some default timeout, etc?
Not currently, I've added a TODO.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@73
PS30, Line 73: // returned.
> it's probably worth a class-wide comment on what the errors returned will b
I've added a TODO. I think we should have test coverage if we're going to document it, and I'd like to leave that to a follow-up commit.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: bool cascade = false,
: bool delete_data = true)
> would this be better off as a bitset of flags?
Not in my opinion. What advantage would that have? This method will only be used in tests, per the current design.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121: int32_t max_events,
> do we have any idea of the expected size of these events? wondering if we n
They can be very large for HDFS tables with a lot of partitions. There's currently no way to filter the events to just include Kudu tables, unfortunately.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@124
PS30, Line 124: // Deserializes a JSON encoded table.
> can you add a little more context as to where one would see the JSON-encode
Done
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include <vector>
> Could you at least add the IWYU pragma, please? It should be something lik
sure, I completely forgot pragmas were an option.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@78
PS30, Line 78: IOError
> would RemoteError be better here?
I think exceptions thrown on the remote end will often come through as MetaException, which I have mapped to RuntimeError. Should that be RemoteError instead?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@94
PS30, Line 94: HmsClient::HmsClient(const HostPort& hms_address)
> is there a world in which people run multiple HMS in an HA configuration? If so is that typically done by configuring clients or putting the HMSs behind a proxy or vip?
Yes I think so. The follow-up commit attempts to handle this via a higher-level component (the HmsCatalog class)
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc@53
PS30, Line 53: WARN_NOT_OK(proc->KillAndWait(SIGTERM), "failed to stop the Hive MetaStore process");
> curious why the graceful shutdown is necessary here vs SIGKILL that we typi
I don't think it's a necessity thing. SIGTERM appears to work reliably, is there a reason to prefer SIGKILL?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc@99
PS30, Line 99: string aux_jars = Substitute("$0/hcatalog/share/hcatalog,$1/hms-plugin.jar",
> can we somehow check that the hms-plugin exists before running this? otherw
It's built as part the cmake build process, so I think we can reliably assume it's available.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 14:54:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#8).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
21 files changed, 2,475 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/8
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/4/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 29: #include "kudu/hms/mini_hms.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7053/4/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 31: #include <limits>
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#32).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,924 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/32
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 32
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#7).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
21 files changed, 2,472 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/7
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 714: -DWITH_BOOSTTHREADS=OFF \
> Yes, we may need to go back and turn this on later, but for now it's simple
Why "may need to"? Now that we're encrypting server-to-server communication, isn't encryption of traffic to HMS a requirement?
Anyway I'm fine with deferring it to later, but we should probably put a big red sign up somewhere. Maybe a JIRA.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#27).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,915 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/27
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#2).
Change subject: WIP: Add basic Hive MetaStore client
......................................................................
WIP: Add basic Hive MetaStore client
This is a work-in-progress patch to create a basic HiveMetastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependency.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
WIP because it's not yet expected to work with dist-test (Hive and
Hadoop artifacts need to be distributed).
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_util.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 2,158 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/2
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed a vote on this change.
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Reviewed-on: http://gerrit.cloudera.org:8080/7053
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 38
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#33).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,926 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/33
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 33
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 27:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
PS24:
> A possible alternative handling the necessity to refresh this file when swi
So I've actually gone back the other way and added a note that this file shouldn't be updated, because to do so implies that a breaking change is being made. We should be very careful about doing that, since we should remain compatible with a range of HMS versions.
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h@98
PS24, Line 98: Status AlterTable(const std::string& database_name,
: const std::string& table_name,
: const hive::Table& table)
> nit: does it make sense to add WARN_UNUSED_RESULT here as well?
Done
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h@108
PS24, Line 108: database
> nit: here and in GetAllTables() -- consider using 'database_name' to match
Done
http://gerrit.cloudera.org:8080/#/c/7053/25/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/25/src/kudu/hms/hms_client.cc@99
PS25, Line 99: : client_(nullptr) {
> Can we use std::make_shared here?
Done
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@125
PS24, Line 125: 'datanucleus.schem
> Which options is this referring to? datanucleus.schema.autoCreateAll and hi
Done
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/build-definitions.sh@728
PS24, Line 728: mkdir -p $THRIFT_BDIR
: pushd $THRIFT_BDIR
: rm -Rf CMakeCache.txt CMakeFiles/
:
: # Thrift
> paranoid nit: instead, maybe
I ended up changing this per Adar's suggestion.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 23:52:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#18).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 2,997 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/18
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#6).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_util.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/thrift-disable-tlsv1_1-openssl-1_0_0.patch
M thirdparty/vars.sh
24 files changed, 2,531 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/6
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128: env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/apache-hive-*-bin"))[0]
: env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0]
when we end up revving these dependencies it seems likely we'll end up with multiple ones in src/. Could we do use the installed/opt/ symlink that you make elsewhere for this?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt@40
PS30, Line 40: execute_process(COMMAND ln -nsf
: "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hive"
: "${EXECUTABLE_OUTPUT_PATH}/hive-home")
: execute_process(COMMAND ln -nsf
: "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop"
: "${EXECUTABLE_OUTPUT_PATH}/hadoop-home")
: execute_process(COMMAND ln -nsf
: "${JAVA_HOME}"
: "${EXECUTABLE_OUTPUT_PATH}/java-home")
does it make more sense to do these in build-thirdparty? even though it's not a "build" step it seems to fit a little closer there (at least for the hive/hadoop ones, not necessarily the java one)
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift@21
PS30, Line 21: # DO NOT MODIFY! Copied from
should we be setting any C++ specific options like 'moveable types'?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc@103
PS30, Line 103: ASSERT_TRUE(CreateTable(&client, database_name, table_name, "").IsRuntimeError());
i think it's worth ASSERT_STR_MATCHES the error string here since RuntimeError is so generic
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@60
PS30, Line 60: // can not be reached, an error is returned.
is there some default timeout, etc?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@73
PS30, Line 73: // returned.
it's probably worth a class-wide comment on what the errors returned will be for other types of issues, such as if the RPC times out or the HMS has disconnected, etc.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: bool cascade = false,
: bool delete_data = true)
would this be better off as a bitset of flags?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121: int32_t max_events,
do we have any idea of the expected size of these events? wondering if we need to be thinking about memory consumption of this component carefully or not
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@124
PS30, Line 124: // Deserializes a JSON encoded table.
can you add a little more context as to where one would see the JSON-encoded table format?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@78
PS30, Line 78: IOError
would RemoteError be better here?
Is there any more specific error to distinguish network issues and timeouts vs exceptions thrown on the remote end?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@94
PS30, Line 94: HmsClient::HmsClient(const HostPort& hms_address)
is there a world in which people run multiple HMS in an HA configuration? If so is that typically done by configuring clients or putting the HMSs behind a proxy or vip?
Do we need to support auto-reconnect after failure? eg if someone just bounces the HMS how does the Kudu master know to reconnect/retry? Is that assumed to be at a higher layer? Might be worth documenting this at the class-doc level.
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@96
PS30, Line 96: auto socket = make_shared<TSocket>(hms_address.host(), hms_address.port());
no need to set timeouts on the TSocket?
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@99
PS30, Line 99: client_ = hive::ThriftHiveMetastoreClient(std::move(protocol));
is there any way we can hook client_ to get a before/after hook on each call? would be great if we could capture the calls as some kind of metric, add things like AssertWaitAllowed, automatically SCOPED_LOG_SLOW_EXECUTION, TRACE_EVENT, etc
Seems like maybe writeMessageBegin() and readMessageEnd() on the protocol level could be a good spot for this, but maybe there is a way to use a thrift plugin or something to get a better hook without having to override TProtocol
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc@53
PS30, Line 53: WARN_NOT_OK(proc->KillAndWait(SIGTERM), "failed to stop the Hive MetaStore process");
curious why the graceful shutdown is necessary here vs SIGKILL that we typically do for our other processes
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/mini_hms.cc@99
PS30, Line 99: string aux_jars = Substitute("$0/hcatalog/share/hcatalog,$1/hms-plugin.jar",
can we somehow check that the hms-plugin exists before running this? otherwise we'll get an annoying java exception trace to dig through right?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:34:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed Tidy Bot from this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Removed reviewer Tidy Bot.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#5).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_util.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/thrift-disable-tlsv1_1-openssl-1_0_0.patch
M thirdparty/vars.sh
24 files changed, 2,529 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/5
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 37: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 37
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 00:07:51 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 31:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG@4
PS31, Line 4: dan@danburkert.com
revert
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 14:56:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.cc@121
PS29, Line 121: return WaitForTcpBind(hms_process_->pid(), &port_, MonoDelta::FromSeconds(20));
Any particular reason this timeout shouldn't be ridiculously high? Like, 90s? Do we have any tests that need to wait for it to fully elapse?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: BISON_NAME=bison-$BISON_VERSION
> It's a SHA from the upstream Apache git repository. Can we not assume cont
Every other commit hash in this file also includes a link to a repository, no matter how trivially accessible. I don't see why we should make an exception for Hive (or Thrift) when it's so easy to add a one line URL to the comment.
Put another way, can you justify these exceptions, or explain why adding these URLs doesn't sit with you?
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/26/thirdparty/vars.sh@203
PS26, Line 203: # TODO(dan): bump to 0.11 when it's released. We chose to use a bleeding edge
> The Apache project page is linked to in the license file. The apache git r
You could link to the github mirror: https://github.com/apache/thrift. That would be fine too.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 17:48:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#36).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/36
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> I'll take a look at that.
Indeed, it seems IWYU is confused, and I cannot see why this happens.
As a temporary workaround, please add path to this file into the 'muted' in $KUDU_ROOT/build-support/iwyu/iwyu-filter.awk
...
muted["kudu/hms/hms_client.cc"]
...
I'll deal with it a bit later.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:35:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc@25
PS29, Line 25: #include <glog/logging.h>
: #include <glog/stl_logging.h>
> stl_logging.h is necessary for the stream formatting on line 194. I'll rem
It seems stl_loggin.h is a frequent point of confusion for IWYU. I'll take a look whether it's possible to handle it with a mapping.
Meanwhile, could you add IWYU pragma, please:
#include <glog/stl_logging.h> // IWYU pragma: keep
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include <vector>
> perhaps, but I don't think I should remove it.
Could you at least add the IWYU pragma, please? It should be something like:
#include <vector> // IWYU pragma: keep
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:41:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 24: Code-Review+1
I can't make heads or tails of the remaining IWYU suggestions.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 02:00:16 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#21).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,881 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/21
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 21
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#15).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 3,001 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/15
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:
http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128: env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/apache-hive-*-bin"))[0]
: env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0]
> Done
Woops, turns out this was correct the first time. dist_test.py ensures that only the correct versions of hive and hadoop are copied, because they use the build/latest/bin/[hadoop-home|hive-home] symlinks.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:15:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(52 comments)
http://gerrit.cloudera.org:8080/#/c/7053/5/build-support/dist_test.py
File build-support/dist_test.py:
Line 82: # Hive MetaStore tests require hadoop and hive libraries.
These directories will be recursively copied? Or are these just symlinks? Can you add to the comment?
http://gerrit.cloudera.org:8080/#/c/7053/5/build-support/run_dist_test.py
File build-support/run_dist_test.py:
Line 127: env['HIVE_HOME'] = os.path.abspath(glob.glob("../../thirdparty/src/apache-hive-*-bin")[0])
Can you use ROOT to derive the paths instead of relying on the cwd?
Line 129: env['JAVA_HOME'] = glob.glob("/usr/lib/jvm/java-1.8.0-*")[0]
Can we run detect-java-home.sh from bigtop instead? This seems rather fragile.
http://gerrit.cloudera.org:8080/#/c/7053/5/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
Line 19: # https://github.com/apache/bigtop/blob/38e1571b2f73bbfa6ab0c01a689fae967b8399d9/bigtop-packages/src/common/bigtop-utils/bigtop-detect-javahome
If this is a specific version for a reason, please explain why. IIRC, it's because this was the last version to support JDK7?
http://gerrit.cloudera.org:8080/#/c/7053/5/cmake_modules/FindThrift.cmake
File cmake_modules/FindThrift.cmake:
PS5, Line 106: #list(APPEND ${SRCS} "${THRIFT_CC_OUT}")
: #list(APPEND ${HDRS} "${THRIFT_H_OUT}")
Remove this?
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
PS5, Line 21: set(HMS_THRIFT_LIBS
: thrift)
This is only used in one place, just inline it there?
Line 23: ADD_EXPORTABLE_LIBRARY(hms_thrift
Is hms_thrift going to be linked into the Kudu client library? If not, it doesn't need to be an exportable library; add_library() and target_link_library() should suffice.
Line 35: ADD_EXPORTABLE_LIBRARY(hms
Likewise.
Line 44: ADD_CUSTOM_TARGET(hive_link_target ALL
Nit: use lowercase for cmake built-ins (i.e. add_custom_target(...)).
Line 49: ADD_CUSTOM_TARGET(hadoop_link_target ALL
So this means the symlinks will be recreated on every invocation of "make" that doesn't include a specific target, or an invocation of "make all".
Isn't that overkill though? Wouldn't it be sufficient to create the symlink on the first run of cmake? You could do that via execute_process().
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 23: // See https://stackoverflow.com/questions/1156003/c-namespace-collision-with-gtest-and-boost
But we're not explicitly including boost here. Is this from ThriftHiveMetastore.h? If so, could we add the workaround directly to the generated header?
Line 42: TEST_F(HiveMetastoreTest, TestCreateDatabase) {
Also test that Stop() works?
Line 52: ASSERT_EQ(vector<string>({ "default" }), databases)
Surprised the compiler needs the vector<string> part and isn't able to figure out the type of the initializer list itself.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore.cc
File src/kudu/hms/hive_metastore.cc:
Line 49: #define HMS_RET_NOT_OK(call, msg) \
Are these all of the exception classes that may be thrown? Is there some way to specify a catch-all in case we're wrong?
Line 63: : client_(nullptr) {
So client_ is actually a pointer? Is the pointer hidden behind a typedef or something?
Line 71: CHECK_OK(Stop());
Maybe WARN_NOT_OK() instead? I think that's the convention we use.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore.h
File src/kudu/hms/hive_metastore.h:
Line 33: // A client for the Hive MetaStore.
Maybe HiveMetastoreClient then? Or HMSClient? HiveMetastore suggests the actual server-side component.
Line 39:
What happens if Start or Stop are called multiple times, or if Stop is called before Start? Is that something we need to protect against?
Line 51: Status CreateDatabase(const Apache::Hadoop::Hive::Database& db);
Forward declare and maybe avoid some includes?
Line 57: Apache::Hadoop::Hive::ThriftHiveMetastoreClient client_;
Guess we can't forward declare this, so maybe forward declaring the above isn't useful.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms-test.cc
File src/kudu/hms/mini_hms-test.cc:
Line 24: class MiniKdcTest : public KuduTest {};
MiniHmsTest
Line 26: TEST_F(MiniKdcTest, TestBasicOperation) {
Shouldn't we also test some sort of RPC?
Oh I see, that's what hive_metastore-test.cc is for. But, I still think we should do something here to verify that the HMS is indeed running properly. Maybe we can look for the presence of a file? Or read its contents?
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 20: #include <csignal>
Don't need?
Line 27: #include "kudu/gutil/strings/split.h"
Don't need?
Line 28: #include "kudu/gutil/strings/strip.h"
Don't need?
Line 33: #include "kudu/util/stopwatch.h"
Don't need?
Line 47: proc->KillAndWait(SIGKILL);
WARN_NOT_OK() here?
Line 60: if (env == nullptr) {
Use a ternary?
Line 63: home_dir = env;
Extra space here.
Line 65: CHECK(Env::Default()->FileExists(home_dir)) << env_var << " not found at " << home_dir;
Shouldn't we gracefully return this sort of error via Status?
Also, if this is a symlink, could we make sure it's not broken? For example, if I upgrade my JVM so my java-home symlink is now broken, would be good to catch that before trying to run Hive.
Line 72: //SCOPED_LOG_SLOW_EXECUTION(WARNING, 10000, "Starting HMS");
Remove? Or uncomment?
Line 77: Env* env = Env::Default();
How about asking users to supply this in the class constructor?
Line 106: int rc = 0;
Nit: do you actually need to initialize the value? Would prefer to adhere to the calling convention of "value is guaranteed to be set if function does not fail".
Line 109: return Status::RuntimeError("schematool failed");
Maybe include the exit status? Should use GetExitStatus() anyway.
Line 112: // Start the Kerberos HMS.
Kerberos HMS?
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/security/test/mini_kdc.cc
File src/kudu/security/test/mini_kdc.cc:
Line 151: RETURN_NOT_OK(kdc_process_->WaitForUdpBind(&options_.port, MonoDelta::FromSeconds(1)));
This seems like a rather short timeout. The old code was even more aggressive; any reason not to wait longer (and perhaps avoid some future flakiness in doing so)?
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 749: return this->WaitForBind(port, "4TCP", timeout);
Nit: remove unnecessary this prefixes, here and below.
Line 756: Status Subprocess::WaitForBind(uint16_t* port, const char* kind, MonoDelta timeout) {
I don't really think this or GetExecutablePath belong in util/subprocess. They don't strike me as functionality we'd want in production, so test_util feels like a better home. AFAICT the only reason to put WaitForBind() in util/subprocess is to get access to the pid, but pid() is already exposed in the subprocess API.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
Line 205: // Attempts to find the path the specified executable, searching the provided
"to the specified executable"
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
Line 27:
New includes not needed?
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
Line 620: Source: http://www.boost.org/
Copy/pasta
Line 658: Source: http://www.python.org/
Copy/pasta?
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 501: CXXFLAGS="$EXTRA_CPPFLAGS $OPENSSL_CFLAGS" \
Isn't this wrong though? Curl is a C program so I'd expect CXXFLAGS has no effect.
Line 670: # Build the date_time boost lib.
Update/remove this.
Line 671: ./bootstrap.sh --prefix=$PREFIX threading=multi --with-libraries=date_time,thread
Is this actually needed? I don't see a corresponding change to FindKuduBoost.cmake.
Oh, is it only for Thrift? If so, could you add a comment? Could you double check that it's indeed needed?
PS5, Line 698: -I$PREFIX/include -I$THRIFT_BDIR/lib/cpp/src
Why do we need these two additions? I can take my answer in the form of a code comment.
Line 732: cp $THRIFT_SOURCE/contrib/fb303/if/fb303.thrift $PREFIX/share/fb303/if
Should be cp -f, right? Otherwise will it overwrite?
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 323: if [ ! -d "$THRIFT_SOURCE" ]; then
No autoreconf?
Line 331: if [ ! -d "$BISON_SOURCE" ]; then
No autoreconf?
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
Perhaps we should follow our LLVM tarball's lead and remove it a priori, to avoid downloading this unnecessary stuff in the first place?
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/vars.sh
File thirdparty/vars.sh:
Line 180: BISON_VERSION=2.7.1
Why use this version of bison? The latest is 3.0.4 and it's quite old already (~2 years).
Line 185: HIVE_NAME=apache-hive-$HIVE_VERSION-bin
Why not just hive-$HIVE_VERSION?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: bool cascade = false,
: bool delete_data = true)
> I'm generally against bool parameters for readability. I liked this C++ tip
Done
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121: int32_t max_events,
> I guess there is no way to limit the request by a certain size? eg we might
No, right now the interface only allows you to set the maximum number of events. I'd expect we can set this pretty low, at 20 or 100 events and be able to keep pace with the HMS. I'll look into what Sentry HA sets their limit to.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Nov 2017 15:15:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#26).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,912 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/26
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 26
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#35).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/35
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 35
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 77: Env* env = Env::Default();
> Not necessarily: when used from KuduTest, callers can pass env_ instead.
Regardless, I don't think this change would make the code better. I believe keeping it consistent with the other mini-* components is valuable. I'm not sure what a (non-theoretical) benefit of injecting the environment into the class would be.
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> It's not an issue of forcing; we could consume unmodified LLVM tarballs and
~200MiB
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#10).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
21 files changed, 2,476 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/10
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS12, Line 327: Status WaitForBind(pid_t pid, uint16_t* port, const char* kind, MonoDelta timeout) {
: // In general, processes do not expose the port they bind to, and
: // reimplementing lsof involves parsing a lot of files in /proc/. So,
: // requiring lsof for tests and parsing its output seems more
: // straight-forward. We call lsof in a loop since it typically takes a long
: // time for it to initialize and bind a port.
:
: string lsof;
: RETURN_NOT_OK(GetExecutablePath("lsof", {"/sbin", "/usr/sbin"}, &lsof));
:
: const vector<string> cmd = {
: lsof, "-wbnP", "-Ffn",
: "-p", std::to_string(pid),
: "-a", "-i", kind
: };
:
: MonoTime deadline = MonoTime::Now() + timeout;
: string lsof_out;
:
: for (int64_t i = 1; ; i++) {
: lsof_out.clear();
: Status s = Subprocess::Call(cmd, "", &lsof_out);
:
: if (s.ok()) {
: StripTrailingNewline(&lsof_out);
: break;
: }
: if (deadline < MonoTime::Now()) {
: return s;
: }
:
: SleepFor(MonoDelta::FromMilliseconds(i * 10));
: }
:
: // The '-Ffn' flag gets lsof to output something like:
: // p19730
: // f123
: // n*:41254
: // The first line is the pid. We ignore it.
: // The second line is the file descriptor number. We ignore it.
: // The third line has the bind address and port.
: vector<string> lines = strings::Split(lsof_out, "\n");
: int32_t p = -1;
: if (lines.size() != 3 ||
: lines[2].substr(0, 3) != "n*:" ||
: !safe_strto32(lines[2].substr(3), &p) ||
: p <= 0) {
: return Status::RuntimeError("unexpected lsof output", lsof_out);
: }
: CHECK(p > 0 && p < std::numeric_limits<uint16_t>::max()) << "parsed invalid port: " << p;
: VLOG(1) << "Determined bound port: " << p;
: *port = p;
: return Status::OK();
: }
: } // anonymous namespace
:
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) {
: return WaitForBind(pid, port, "4TCP", timeout);
: }
:
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) {
: return WaitForBind(pid, port, "4UDP", timeout);
: }
> I thought you were about to move this into a separate changelist.
woops, that's probably because this changelist hasn't been yet re-based.
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:
PS12, Line 114: // as well as the $PATH environment variable.
: Status GetExecutablePath(const std::string& binary,
: const std::vector<std::string>& search,
: std::string* path) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening TCP port, and returns the port.
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening UDP port, and returns the port.
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
> I thought you were about to move this into a separate changelist.
woops, that's probably because this changelist hasn't been yet re-based.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#4).
Change subject: Add basic Hive MetaStore client
......................................................................
Add basic Hive MetaStore client
This patch lays the groundwork for a Hive metastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_util.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/thrift-disable-tlsv1_1-openssl-1_0_0.patch
M thirdparty/vars.sh
24 files changed, 2,529 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/4
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#3).
Change subject: WIP: Add basic Hive MetaStore client
......................................................................
WIP: Add basic Hive MetaStore client
This is a work-in-progress patch to create a basic HiveMetastore client.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Hive and Hadoop have been added to thirdparty as test-only dependency.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
WIP because it's not yet expected to work with dist-test (Hive and
Hadoop artifacts need to be distributed).
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore-test.cc
A src/kudu/hms/hive_metastore.cc
A src/kudu/hms/hive_metastore.h
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/mini_hms-test.cc
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/security/test/mini_kdc.cc
M src/kudu/security/test/mini_kdc.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M src/kudu/util/test_util.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
21 files changed, 2,157 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/3
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 36: Code-Review+1
Will defer the +2 to Todd, but all of my issues have been addressed, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:07:57 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#29).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,911 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/29
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#28).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M src/kudu/util/test_util.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,915 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/28
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 28
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 10:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/7053/10/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
PS10, Line 55: /Library/Java/Home
> Just FYI: at my laptop JDK is in /Library/Java/JavaVirtualMachines/jdk1.8.0
I've changed this to use /usr/libexec/java_home on OS X, which should always give the correct answer.
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
PS10, Line 47: COMMAND ln -sf
: "${JAVA_HOME}"
: "${EXECUTABLE_OUTPUT_PATH}/java-home"
> As I understand, ${JAVA_HOME} is a directory. What happens if this command
Done
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
PS10, Line 60: }
> Does it make sense to add catch-all as well?
The TException acts as a catchall (in addition to handling network errors).
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
PS10, Line 50: ;
> Consider adding WARN_UNUSED_RESULT
Done
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
PS10, Line 26: #include <gflags/gflags.h>
> Is it needed in here?
Done
PS10, Line 49: SIGKILL
> Why not SIGTERM?
Done
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
PS10, Line 40:
> const ?
Done
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:
PS10, Line 112: // Attempts to find the path to the executable, searching the provided locations
: // as well as the $PATH environment variable.
: Status GetExecutablePath(const std::string& binary,
: const std::vector<std::string>& search,
: std::string* path) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening TCP port, and returns the port.
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening UDP port, and returns the port.
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
> Maybe, it's worth publishing it ((.e. moving those things from kerberos-sp
Good idea.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 77: Env* env = Env::Default();
> Why is it better? It forces every caller to call Env::Default() instead.
Not necessarily: when used from KuduTest, callers can pass env_ instead.
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> We are forced to do that because LLVM doesn't a unified tarball with the co
It's not an issue of forcing; we could consume unmodified LLVM tarballs and "reassemble" the source tree locally.
How much space do the docs consume post-compression?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Patch Set 16:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
PS12, Line 67: ASSERT_OK(client.GetDatabase(dat
> nit here and below: may be, output the whole info on those objects only if
Done
http://gerrit.cloudera.org:8080/#/c/7053/16/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
Line 32: #include "kudu/hms/hive_metastore_constants.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
PS12, Line 49: // as a catch all, as well as default for network errors.
: #define HMS_RET_NOT_OK(call, msg) \
: try { \
: (call); \
> nit: not very important here, but anyway it might make sense to add parenth
Done
http://gerrit.cloudera.org:8080/#/c/7053/16/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
Line 31: #include "kudu/hms/hive_metastore_constants.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
PS12, Line 38: NotificationLogTtl(Mono
> nit: the std::unique_ptr has specific operator
Done
http://gerrit.cloudera.org:8080/#/c/7053/16/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
Line 55: Status CreateHiveSite(const std::string& dir) const WARN_UNUSED_RESULT;
> warning: function 'kudu::MiniHms::CreateHiveSite' has a definition with dif
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 23: // See https://stackoverflow.com/questions/1156003/c-namespace-collision-with-gtest-and-boost
> Then it would be leaking into non-test code. It's also harder because it's
Perhaps you can encapsulate the workaround in a new test-only header then?
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 77: Env* env = Env::Default();
> Why? This isn't how mini_kdc works.
I know, nor is it how ExternalMiniCluster works, but that doesn't mean we can't do better (especially when it's easy).
Line 109: return Status::RuntimeError("schematool failed");
> I'm skeptical that anyone can glean information from the exit status of sch
Perhaps, but "exit status" is a real thing that I know how to correlate to other stuff (i.e. I can look at schematool and see what values it exits with). This return code from Wait comes straight from waitpid() and is several different things combined.
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 732: cp $THRIFT_SOURCE/contrib/fb303/if/fb303.thrift $PREFIX/share/fb303/if
> We don't use -f anywhere else in this file. Could you elaborate on why it'
I thought that on some distros, cp without -f won't overwrite if the destination file already exists (and may prompt for action). Perhaps not.
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 323: if [ ! -d "$THRIFT_SOURCE" ]; then
> autoreconf is failing on OS X.
Alright, but you're on the hook for any obscure autotools version mismatch related breakages.
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> We don't do that for LLVM. https://github.com/apache/kudu/blob/master/thir
What I meant was: we're comfortable building the combined LLVM tarball ourselves, so let's use that as inspiration and modify our uploaded hadoop tarball to excise the documentation.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> ~200MiB
That's a lot. By the way of comparison, the unified LLVM tarball (which I believe is the largest dependency right now) is 50 MB. So I do think we should remove these docs from our mirrored Hadoop tarball.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 6:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/7053/6//COMMIT_MSG
Commit Message:
Line 9: This patch lays the groundwork for a Hive metastore client.
any progress on the design doc for this work? Would be good to be able to refer to it side-by-side with the code. (even though this is just prep work and doesn't implement much in the way of a design, I think the context would be useful for people following along)
http://gerrit.cloudera.org:8080/#/c/7053/6/CMakeLists.txt
File CMakeLists.txt:
Line 829: find_package(JavaHome REQUIRED)
do we need to update any build docs to specify that java is now a requirement even if you don't plan to build the java client?
http://gerrit.cloudera.org:8080/#/c/7053/6/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
Line 64: set(JAVA_HOME_FOUND true)
can we do something here to check that EXISTS ${JAVA_HOME}/bin/java just in case the user has set some garbage value?
http://gerrit.cloudera.org:8080/#/c/7053/6/cmake_modules/FindThrift.cmake
File cmake_modules/FindThrift.cmake:
PS6, Line 105: # TODO(dan): Add the fb303 files manually. This is a complete hack.
: #list(APPEND ${SRCS} "${THRIFT_CC_OUT}")
: #list(APPEND ${HDRS} "${THRIFT_H_OUT}")
:
hrm, could we at least add this as an optional argument like REQUIRE_FB303?
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
Line 23: ADD_EXPORTABLE_LIBRARY(hms_thrift
since the client won't use hms at all, I think we can use the built-in ADD_LIBRARY instead
Line 35: ADD_EXPORTABLE_LIBRARY(hms
same
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 53: << "Databases: " << JoinStrings(databases, ", ");
I think if you include stl_logging.h then you don't need to do this << part - gtest should be able to automatically log the vector<string>
Line 61: ASSERT_EQ(vector<string>({ "default", "my_db" }), databases)
is there a gaurantee that they come back in sorted order or is a std::sort() above a good idea?
Line 62: << "Databases: " << JoinStrings(databases, ", ");
same
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore.cc
File src/kudu/hms/hive_metastore.cc:
PS6, Line 53: "failed to "
compared to the other macros we have like RETURN_NOT_OK_PREPEND, it's odd to see "failed to" here instead of in the actual usage sites
PS6, Line 64: boost::shared_ptr<TSocket> socket(new TSocket(hms_address.host(), hms_address.port()));
: boost::shared_ptr<TTransport> transport(new TBufferedTransport(socket));
: boost::shared_ptr<TProtocol> protocol(new TBinaryProtocol(transport));
: client_ = Apache::Hadoop::Hive::ThriftHiveMetastoreClient(protocol);
Do any of these ctors potentially throw? (aside from OOM type cases which we dont care about)
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/hive_metastore.h
File src/kudu/hms/hive_metastore.h:
Line 40: // Starts the Hive MetaStore client.
can you specify what this is doing under the hood a little bit more? eg does this actually resolve and connect? (so the caller knows whether to expect (a) it might be slow and (b) it might fail if the HMS is down)
PS6, Line 51: Status CreateDatabase(const Apache::Hadoop::Hive::Database& db);
I think GSG discourages namespace aliases, but maybe we should make an exception here and add something like: 'namespace hive = Apache::Hadoop::Hive' in this header file? It might get a bit tedious to retype this long namespace
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 65: home_dir = env;
nit: extra space
Line 91: LOG(INFO) << "db_path: " << db_path;
vlog?
PS6, Line 114: Kerberos
copy pasta
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
PS6, Line 760: We call lsof in a loop since it typically takes a long
: // time for it to initialize and bind a port.
this was a bit specific to the KDC case, I think could be cleaned up a bit.
http://gerrit.cloudera.org:8080/#/c/7053/6/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
PS6, Line 160: bind to listening TCP por
"bind to ANY listening..."?
PS6, Line 205: path the
typo
PS6, Line 206: // locations as well as the environment path.
you mean the $PATH environment variable?
http://gerrit.cloudera.org:8080/#/c/7053/6/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
Line 620: Source: http://www.boost.org/
wrong paste?
Line 658: Source: http://www.python.org/
same
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 19:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/21/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/21/src/kudu/hms/CMakeLists.txt@38
PS21, Line 38: # hms-test
this should be mini_hms
http://gerrit.cloudera.org:8080/#/c/7053/19/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/19/src/kudu/hms/hms_client.cc@34
PS19, Line 34: #include "kudu/hms/hive_metastore_constants.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 19
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:36:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 27:
Dissapointingly, the 10s limit on starting up the mini hms was too aggressive causing flakiness, so I bumped it to 20s. No failures in 1000 runs.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 27
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 03:38:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#34).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/34
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 34
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client-test.cc@25
PS29, Line 25: #include <glog/stl_logging.h>
: #include <gtest/gtest.h>
> I think IWYU is right about removing these; I don't see any LOG/VLOG usage
stl_logging.h is necessary for the stream formatting on line 194. I'll remove glog/logging.h.
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include <vector>
> Maybe IWYU wants to remove this because:
perhaps, but I don't think I should remove it.
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> Indeed, it seems IWYU is confused, and I cannot see why this happens.
Done
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> IWYU's recommendation to remove this makes sense: you can forward declare S
unique_ptr doesn't work with forward declared classes.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 14:05:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 23: // See https://stackoverflow.com/questions/1156003/c-namespace-collision-with-gtest-and-boost
> Perhaps you can encapsulate the workaround in a new test-only header then?
I want to punt on this until we have two cases where it's necessary, I'm still not 100% sure of exactly the header order necessary to have this issue.
http://gerrit.cloudera.org:8080/#/c/7053/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 77: Env* env = Env::Default();
> I know, nor is it how ExternalMiniCluster works, but that doesn't mean we c
Why is it better? It forces every caller to call Env::Default() instead.
Line 109: return Status::RuntimeError("schematool failed");
> Perhaps, but "exit status" is a real thing that I know how to correlate to
I've changed it to get the real exit code, but I'm still not printing it because I don't think it's useful. schema tool and hive loudly log errors.
http://gerrit.cloudera.org:8080/#/c/7053/5/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 323: if [ ! -d "$THRIFT_SOURCE" ]; then
> Alright, but you're on the hook for any obscure autotools version mismatch
I've switched thrift to use cmake instead. Bison continues to not call autoreconf since I can't get it to work on all platforms.
Line 342: # Remove the share/doc dir which contains 2GiB of HTML project documentation.
> What I meant was: we're comfortable building the combined LLVM tarball ours
We are forced to do that because LLVM doesn't a unified tarball with the components we need. I'd prefer not to mess with tarballs when we don't need to, since it makes it much easier to trace provenance. The .html documents compress fairly well, so the actual tarball isn't inflated by 2GiB.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: WIP: Add basic Hive MetaStore client
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/2/src/kudu/hms/hive_metastore-test.cc
File src/kudu/hms/hive_metastore-test.cc:
Line 29: #include "kudu/hms/mini_hms.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7053/2/src/kudu/hms/hive_metastore.cc
File src/kudu/hms/hive_metastore.cc:
Line 87: HMS_RET_NOT_OK(client_.create_database(db), "create Hive MetaStore database");
> warning: passing result of std::move() as a const reference argument; no mo
Done
http://gerrit.cloudera.org:8080/#/c/7053/2/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 50:
> warning: using decl 'vector' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/7053/2/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:
Line 768: break;
> warning: do not use 'else' after 'break' [readability-else-after-return]
I hate this rule.
Line 772: }
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done
http://gerrit.cloudera.org:8080/#/c/7053/2/src/kudu/util/subprocess.h
File src/kudu/util/subprocess.h:
Line 201: Status GetExecutablePath(const std::string& binary,
> warning: function 'kudu::GetExecutablePath' has a definition with different
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: BISON_NAME=bison-$BISON_VERSION
> Every other commit hash in this file also includes a link to a repository,
That’s just the thing, though, none of those other dependencies are trivial to find.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 23:01:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> You sure about that? I'm looking at https://stackoverflow.com/questions/601
Sorry for not responding before. This appears to me to be toolchain dependent (probably libstdcxx vs libcxx), because this won't compile on macos (libcxx) with a forwards declare. I'm not crazy about the idea of forgoing the simplicity of '= default;' just to satisfy IWYU, so I'd like to keep it as is.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:06:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 24:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@101
PS24, Line 101: map<string, string> env_vars {
> Why should this variable in particular be const?
Since it's not intended to change in the scope, it might make sense to mark in const. It's just a nit, feel free to ignore.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:12:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#17).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 3,002 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/17
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 22:
(17 comments)
I focused mostly on the glue, since I think I (and Todd) already reviewed the client.
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt@870
PS22, Line 870: find_package(Java 1.7 REQUIRED)
Could you add a comment that the version argument is >=, not == ?
http://gerrit.cloudera.org:8080/#/c/7053/22/CMakeLists.txt@871
PS22, Line 871: include(UseJava)
Can you add a comment saying that this allows us to call add_jar()?
http://gerrit.cloudera.org:8080/#/c/7053/22/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
http://gerrit.cloudera.org:8080/#/c/7053/22/cmake_modules/FindJavaHome.cmake@74
PS22, Line 74: # Use the 'java_home' finder on macOS.
Nit: I think it'd be clearer if this were inverted. That is, if this were under a if (APPLE) case.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@31
PS22, Line 31: kudu_common
Why is this needed?
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@57
PS22, Line 57: "${CMAKE_SOURCE_DIR}/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java"
Can you use SOURCES and then glob this directory?
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@59
PS22, Line 59: OUTPUT_DIR "${EXECUTABLE_OUTPUT_PATH}")
What does EXECUTABLE_OUTPUT_PATH resolve to? Where does the JAR end up?
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@64
PS22, Line 64: add_library(mini_hms ${MINI_HMS_SRCS})
If this is to be used by the minicluster CLI tool, it can't be conditioned on NOT NO_TESTS.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@73
PS22, Line 73: hms
FWIW, 'hms' and 'mini-hms' side-by-side suggest that both are metastore implementations. Perhaps rename 'hms' to something like 'hms_util' or 'hms_client'?
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/hive_metastore.thrift@22
PS22, Line 22: # https://raw.githubusercontent.com/apache/hive/rel/release-2.3.0/metastore/if/hive_metastore.thrift
Could you add a note in vars.sh next to Hive that if we bump the version, we ought to replace this file?
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc@125
PS22, Line 125: static const string kFileTemplate = R"(
Would be nice to sprinkle comments here explaining the choice for each non-default option.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
PS22:
What about hadoop and hive?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@726
PS22, Line 726: build_thrift() {
Should mention that this depends on bison. See build_glog for inspiration.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@730
PS22, Line 730: rm -Rf *
Why is this necessary? None of our other dependency builds do this.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@737
PS22, Line 737: cmake \
Add EXTRA_CMAKE_FLAGS and then use ${NINJA:-make} instead of make.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh@343
PS22, Line 343: if [ ! -d "$THRIFT_SOURCE" ]; then
Apparently we should now be setting PATCHLEVEL=0 on new deps to ease future patches.
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/download-thirdparty.sh@349
PS22, Line 349: # This would normally call autoreconf, but it does not succeed on every
: # platform.
Can you add a little more detail? Do you remember which platform failed?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: HIVE_VERSION=3fb4649fa847cfec33f701f6c884f12087680cf0
How was this built? Is it sourced from github? Could you include instructions?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 22
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 21:43:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7053/8/CMakeLists.txt
File CMakeLists.txt:
Line 1144: add_subdirectory(src/kudu/hms)
Nit: resort.
http://gerrit.cloudera.org:8080/#/c/7053/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 64: return Status::IllegalState(Substitute("$0 directory does not exist", env_var), *home_dir);
Status::NotFound() seems more appropriate here.
http://gerrit.cloudera.org:8080/#/c/7053/8/src/kudu/security/test/mini_kdc.cc
File src/kudu/security/test/mini_kdc.cc:
Line 29:
Can you check whether any of these includes are no longer needed now that you've removed those functions from this file?
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 702: cmake \
Did you explicitly enumerate all BUILD/WITH options? Or are all of these non-default values?
Line 714: -DWITH_OPENSSL=OFF \
Does this disable TLS support altogether?
Line 717: -DBUILD_EXAMPLES=OFF \
Nit: resort.
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 325: if [ ! -d "$BISON_SOURCE" ]; then
Could you add a comment here saying that we could run autoreconf but it doesn't seem to work properly on various platforms, so we won't?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has removed a vote on this change.
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 36:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30
PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep
Any particular reason you don't want to move MiniHms constructor into mini_hms.cc, thus allowing you to cleanly forward declare Subprocess and avoid this inclusion? Alexey and I both suggested that back in PS29/PS30.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:12:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 29: Code-Review+1
IWYU is confused.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 23:02:39 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#31).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,920 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/31
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 24:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:
PS24:
A possible alternative handling the necessity to refresh this file when switching to a new version of Hive: maybe, put this file into the Hive distro tarball and install it upon unpacking the apache-hive-$HIVE_VERSION-bin tarball somewhere under thirdparty/installed ?
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h@98
PS24, Line 98: Status AlterTable(const std::string& database_name,
: const std::string& table_name,
: const hive::Table& table);
nit: does it make sense to add WARN_UNUSED_RESULT here as well?
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/hms_client.h@108
PS24, Line 108: database
nit: here and in GetAllTables() -- consider using 'database_name' to match the rest of those methods, or just replace 'database_name' with 'database' elsewhere.
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@101
PS24, Line 101: map<string, string> env_vars {
nit: const ?
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/build-definitions.sh@728
PS24, Line 728: mkdir -p $THRIFT_BDIR
: pushd $THRIFT_BDIR
: # Normally we would only remove cached CMake files, but the Thrift build is
: # flaky on CI unless all build state is removed.
: rm -Rf *
paranoid nit: instead, maybe
rm -Rf $THRIFT_BDIR
mkdir -p $THRIFT_BDIR
pushd $THRIFT_BDIR
?
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/24/thirdparty/vars.sh@211
PS24, Line 211: # When bumping the Hive versions, also update hive_metastore.thrift.
Consider the 'automated' way of updating the hive_metastore.thrift file as described in file comment for thirdparty/vars.sh
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:54:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] Hive MetaStore client, HMS Plugin
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#16).
Change subject: Hive MetaStore client, HMS Plugin
......................................................................
Hive MetaStore client, HMS Plugin
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The primary focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client can create, rename,
and drop Kudu table entries in the HMS, as well as retrieve notification
log events.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- A new Kudu HMS plugin has been added in a new 'kudu-hive' maven
module. This plugin helps ensure that the Kudu master and the HMS
maintain metadata consistency. Since this plugin is used during the
C++ unit tests, it is compiled into a single-class jar as part of the
C++ build.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A java/kudu-hive/pom.xml
A java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java
M java/pom.xml
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
20 files changed, 3,002 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/16
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 30:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> unique_ptr doesn't work with forward declared classes.
You sure about that? I'm looking at https://stackoverflow.com/questions/6012157/is-stdunique-ptrt-required-to-know-the-full-definition-of-t and it suggests that, for most operations incomplete knowledge of the class is OK.
I also wrote a small test that's similar to mini_hms to prove this to myself:
--bar.cc--
#include "bar.h"
#include "foo.h"
Bar::~Bar() {}
void Bar::CreateFoo() {
f.reset(new Foo());
f->a = "hello world";
}
--test.cc--
#include "bar.h"
#include "foo.h"
int main(void) {
Bar b;
b.CreateFoo();
return 0;
}
--bar.h--
#include <memory>
struct Foo;
struct Bar {
Bar() = default;
~Bar();
void CreateFoo();
private:
std::unique_ptr<Foo> f;
};
--foo.h--
#include <string>
struct Foo {
std::string a;
};
This test compiles and links. As you can see, bar.h has only partial knowledge of Foo, but full knowledge of it where needed (bar.cc and test.cc).
Also, if I follow Alexey's advice and move Bar's noarg constructor from bar.h to bar.cc, I can remove the inclusion of foo.h from test.cc.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 19:47:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 10:
(6 comments)
Just skimmed through -- will take another look tomorrow. Overall looks good!
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
PS10, Line 47: COMMAND ln -sf
: "${JAVA_HOME}"
: "${EXECUTABLE_OUTPUT_PATH}/java-home"
As I understand, ${JAVA_HOME} is a directory. What happens if this command run twice? Wouldn't it be a just another link in java-home subdir? Is that true for the first two as well?
If so, consider using 'ln -nsf' instead of 'ln -sf'
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
PS10, Line 26: #include <gflags/gflags.h>
Is it needed in here?
PS10, Line 49: SIGKILL
Why not SIGTERM?
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:
PS10, Line 112: // Attempts to find the path to the executable, searching the provided locations
: // as well as the $PATH environment variable.
: Status GetExecutablePath(const std::string& binary,
: const std::vector<std::string>& search,
: std::string* path) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening TCP port, and returns the port.
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening UDP port, and returns the port.
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
Maybe, it's worth publishing it ((.e. moving those things from kerberos-specific files into the utils) as a separate changelist ? That way it would be easier to review and track changes in the changelog.
http://gerrit.cloudera.org:8080/#/c/7053/10/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:
PS10, Line 237: sf
-nsf ?
PS10, Line 242: sf
-nsf ?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 9:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7053/8/CMakeLists.txt
File CMakeLists.txt:
Line 1144: add_subdirectory(src/kudu/hms)
> Nit: resort.
Done
http://gerrit.cloudera.org:8080/#/c/7053/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
Line 64: return Status::IllegalState(Substitute("$0 directory does not exist", env_var), *home_dir);
> Status::NotFound() seems more appropriate here.
Done
http://gerrit.cloudera.org:8080/#/c/7053/8/src/kudu/security/test/mini_kdc.cc
File src/kudu/security/test/mini_kdc.cc:
Line 29:
> Can you check whether any of these includes are no longer needed now that y
Done
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 702: -DCMAKE_BUILD_TYPE=Release \
> Did you explicitly enumerate all BUILD/WITH options? Or are all of these no
Pretty much the former. For whatever reason thrift defaults everything to ON, so I looked at the list of things it was building and turned everything off I didn't like.
Line 714: -DWITH_PLUGIN=OFF \
> Does this disable TLS support altogether?
Yes, we may need to go back and turn this on later, but for now it's simpler if we don't need to tell thrift where to find OpenSSL.
Line 717: -DBUILD_EXAMPLES=OFF \
> Nit: resort.
Done
http://gerrit.cloudera.org:8080/#/c/7053/8/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 325: if [ ! -d "$BISON_SOURCE" ]; then
> Could you add a comment here saying that we could run autoreconf but it doe
Done
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 12:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7053/10/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
PS10, Line 55: /usr/java/default
> I've changed this to use /usr/libexec/java_home on OS X, which should alway
I see -- that's much better.
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
PS12, Line 67: LOG(INFO) << "my_db: " << my_db;
nit here and below: may be, output the whole info on those objects only if any of the relevant asserts below fail? I'm not a big fan of chatty tests when all is going well. If this chattiness is really useful, that's fine, of course.
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
PS10, Line 60:
> The TException acts as a catchall (in addition to handling network errors).
ah, I see. It's safe then.
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
PS12, Line 49: try { \
: call; \
: } catch (const hive::AlreadyExistsException& e) { \
: return Status::AlreadyPresent(msg, e.what());
nit: not very important here, but anyway it might make sense to add parenthesis around the macro's parameters in the body of the macro.
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
PS12, Line 38: hms_process_ == nullptr
nit: the std::unique_ptr has specific operator
explicit operator bool() const noexcept;
so it might be just
CHECK(!hms_process_);
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:
PS12, Line 327: Status WaitForBind(pid_t pid, uint16_t* port, const char* kind, MonoDelta timeout) {
: // In general, processes do not expose the port they bind to, and
: // reimplementing lsof involves parsing a lot of files in /proc/. So,
: // requiring lsof for tests and parsing its output seems more
: // straight-forward. We call lsof in a loop since it typically takes a long
: // time for it to initialize and bind a port.
:
: string lsof;
: RETURN_NOT_OK(GetExecutablePath("lsof", {"/sbin", "/usr/sbin"}, &lsof));
:
: const vector<string> cmd = {
: lsof, "-wbnP", "-Ffn",
: "-p", std::to_string(pid),
: "-a", "-i", kind
: };
:
: MonoTime deadline = MonoTime::Now() + timeout;
: string lsof_out;
:
: for (int64_t i = 1; ; i++) {
: lsof_out.clear();
: Status s = Subprocess::Call(cmd, "", &lsof_out);
:
: if (s.ok()) {
: StripTrailingNewline(&lsof_out);
: break;
: }
: if (deadline < MonoTime::Now()) {
: return s;
: }
:
: SleepFor(MonoDelta::FromMilliseconds(i * 10));
: }
:
: // The '-Ffn' flag gets lsof to output something like:
: // p19730
: // f123
: // n*:41254
: // The first line is the pid. We ignore it.
: // The second line is the file descriptor number. We ignore it.
: // The third line has the bind address and port.
: vector<string> lines = strings::Split(lsof_out, "\n");
: int32_t p = -1;
: if (lines.size() != 3 ||
: lines[2].substr(0, 3) != "n*:" ||
: !safe_strto32(lines[2].substr(3), &p) ||
: p <= 0) {
: return Status::RuntimeError("unexpected lsof output", lsof_out);
: }
: CHECK(p > 0 && p < std::numeric_limits<uint16_t>::max()) << "parsed invalid port: " << p;
: VLOG(1) << "Determined bound port: " << p;
: *port = p;
: return Status::OK();
: }
: } // anonymous namespace
:
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) {
: return WaitForBind(pid, port, "4TCP", timeout);
: }
:
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) {
: return WaitForBind(pid, port, "4UDP", timeout);
: }
I thought you were about to move this into a separate changelist.
http://gerrit.cloudera.org:8080/#/c/7053/12/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:
PS12, Line 114: // as well as the $PATH environment variable.
: Status GetExecutablePath(const std::string& binary,
: const std::vector<std::string>& search,
: std::string* path) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening TCP port, and returns the port.
: Status WaitForTcpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
:
: // Waits for the subprocess to bind to any listening UDP port, and returns the port.
: Status WaitForUdpBind(pid_t pid, uint16_t* port, MonoDelta timeout) WARN_UNUSED_RESULT;
I thought you were about to move this into a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 24:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@31
PS22, Line 31: kudu_common
> Done
Not done? I still see kudu_common in HMS_DEPS.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/CMakeLists.txt@57
PS22, Line 57: INCLUDE_JARS ${DEPENDENCY_JARS}
> From the CMake docs:
Oh, add_jar() is smart about tracking dependencies on individual source files? Okay then, this makes sense.
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/22/src/kudu/hms/mini_hms.cc@125
PS22, Line 125: // The schema options allow Hive to startup and run without first running the
> Done
Could you also add a comment for hive.metastore.event.db.listener.timetolive? The rest are somewhat self-explanatory, I guess.
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@125
PS24, Line 125: The schema options
Which options is this referring to? datanucleus.schema.autoCreateAll and hive.metastore.schema.verification? Could you call them out specifically to make this more clear?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/build-definitions.sh@730
PS22, Line 730: # Normally we would only remove cached CMake files, but the Thrift build is
> Comment added. This was changed in R17 as a result of failed builds on CI.
OK, but every single rebuild of thirdparty is going to have to rebuild thrift as a result. How long does thrift take to build with a cold ccache?
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/7053/22/thirdparty/vars.sh@212
PS22, Line 212: # TODO(dan): bump to a release version once HIVE-17747 is published.
> Yes it was sourced from the apache hive git repository. We should not be d
OK but could you at least leave a breadcrumb here pointing to the git repository? Would build instructions ala SPARSEHASH or SPARSEPP work?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 21:34:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add basic Hive MetaStore client
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Add basic Hive MetaStore client
......................................................................
Patch Set 10:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7053/10/cmake_modules/FindJavaHome.cmake
File cmake_modules/FindJavaHome.cmake:
PS10, Line 55: /Library/Java/Home
Just FYI: at my laptop JDK is in /Library/Java/JavaVirtualMachines/jdk1.8.0_102.jdk
so JAVA_HOME seems to be
/Library/Java/JavaVirtualMachines/jdk1.8.0_102.jdk/Contents/Home
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
PS10, Line 60: }
Does it make sense to add catch-all as well?
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
PS10, Line 50: ;
Consider adding WARN_UNUSED_RESULT
http://gerrit.cloudera.org:8080/#/c/7053/10/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
PS10, Line 40:
const ?
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#30).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,911 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/30
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 )
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
Patch Set 36:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: in the HMS.
: //
> Not in my opinion. What advantage would that have? This method will only
I'm generally against bool parameters for readability. I liked this C++ tip: https://abseil.io/tips/94
http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121: // Retrieves all tables in an HMS database.
> They can be very large for HDFS tables with a lot of partitions. There's c
I guess there is no way to limit the request by a certain size? eg we might want to get 1000 events normally, but if we cross some threshold like 10MB then stop at that point?
I'm just not sure how we'll appropriately decide on a value for 'max_events' that is both high-throughput and memory-safe. 1000 events is probably fine most of the time, but if we get 1000x 10MB events in some catch-up scenario then all of a sudden we have memory issues to worry about.
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Oct 2017 23:36:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7053
to look at the new patch set (#22).
Change subject: KUDU-2191 (2/n): Hive Metastore client
......................................................................
KUDU-2191 (2/n): Hive Metastore client
This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.
The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.
- Thrift has been added as a dependency, and a mechanism for performing
Thrift codegen at compile time has been added (see FindThrift.cmake,
based on FindProtobuf.cmake)
- Bison has been added as a build-time dependency, because the system
bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.
- Hive and Hadoop have been added to thirdparty as test-only dependencies.
- A Hive MetaStore external mini server is included for testing. See
mini_hms.[h|cc].
- The Kudu Metastore plugin is compiled from CMake as a standalone jar
for loading into the HMS mini server.
Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 2,881 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/22
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 22
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>