Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8121 )
Change subject: KUDU-2191 (1/n): Hive Metastore Kudu plugin ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/8121/2//COMMIT_MSG Commit Message: PS2: > Would be nice to see a short blurb as to why the plugin should live in Kudu Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/build.gradle File java/kudu-hive/build.gradle: http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/build.gradle@19 PS2, Line 19: > Nit: remove empty line. Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/build.gradle@31 PS2, Line 31: systemProperty "testdata.dir", temporaryDir : systemProperty "derby.stream.error.file", "$temporaryDir/derby.log" > Could you add comments explaining why these are necessary? Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/pom.xml File java/kudu-hive/pom.xml: http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/pom.xml@35 PS2, Line 35: <version>2.3.0</version> > Can you define this in the parent pom? Below too. Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/pom.xml@35 PS2, Line 35: <version>2.3.0</version> > Can you define this in the parent pom? Below too. Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/pom.xml@95 PS2, Line 95: <testdata.dir>${testdata.dir}</testdata.dir> : <derby.stream.error.file>${testdata.dir}/derby.log</derby.stream.error.file> > Please add comments explaining why these need to be set. Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/pom.xml@95 PS2, Line 95: <testdata.dir>${testdata.dir}</testdata.dir> : <derby.stream.error.file>${testdata.dir}/derby.log</derby.stream.error.file> > Please add comments explaining why these need to be set. Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java File java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java@46 PS2, Line 46: * The plugin enforces that Kudu table entries in the HMS always : * contain two properties: a Kudu table ID and the Kudu master addresses. It also : * enforces that non-Kudu tables do not have these properties. The plugin : * considers entries to be Kudu tables if they contain the Kudu storage handler. : * : * Additionally, the plugin checks that when particular events have an : * environment containing a Kudu table ID, that event only applies : * to the specified Kudu table. This provides some amount of concurrency safety, : * so that the Kudu Master can ensure it is operating on the correct table entry. > I appreciate this level of detail, but you might consider generalizing a bi I think I want to leave it at this level of specificity. I don't think we are going to have free-reign to update the metadata which we require. In a sense, the format we choose now to store in the HMS is akin to an on-disk format. Keep in mind that while this is a class-level doc, this isn't actually a class which people will use in their own code. The only people reading the class doc are going to be people interested in what the plugin is doing and how it does it. http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@75 PS2, Line 75: private static Table table(String name) { > Since it creates a Table, maybe call it newTable? Done http://gerrit.cloudera.org:8080/#/c/8121/2/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@110 PS2, Line 110: } catch (Exception e) { > Can't catch a Hive-specific exception type? TException (thrift) is the LUB. The throws list is truly outrageous: https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java#L752-L753. http://gerrit.cloudera.org:8080/#/c/8121/2/java/pom.xml File java/pom.xml: http://gerrit.cloudera.org:8080/#/c/8121/2/java/pom.xml@98 PS2, Line 98: <testdata.dir>${project.build.directory}/testdata</testdata.dir> > What is this even used for? I know there's an antrun invocation that delete Not sure about antrun, but we're using it now as a variable substitution in hive-site.xml. http://gerrit.cloudera.org:8080/#/c/8121/2/java/pom.xml@105 PS2, Line 105: <modules> > Does this alphabetical reordering work? Doesn't kudu-client-tools depend on It appears to work fine. kudu-mapreduce is a dependency of kudu-client-tools, so it wasn't topographically ordered before anyway. http://gerrit.cloudera.org:8080/#/c/8121/2/java/settings.gradle File java/settings.gradle: http://gerrit.cloudera.org:8080/#/c/8121/2/java/settings.gradle@21 PS2, Line 21: rootProject.name = "kudu-parent" > Want to reorder these too? Done -- To view, visit http://gerrit.cloudera.org:8080/8121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief0d64f71f90e04588a05098a44658fd461b9ec8 Gerrit-Change-Number: 8121 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 17 Oct 2017 22:37:11 +0000 Gerrit-HasComments: Yes
