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 <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@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-HasComments: Yes
