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 <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 21:43:02 +0000 Gerrit-HasComments: Yes
