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 <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: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 18 Oct 2017 23:52:08 +0000 Gerrit-HasComments: Yes
