Adar Dembo 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: (11 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's repo and not Hive's. 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. 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? 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. 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. 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 bit so that this comment won't need to be updated as the plugin evolves. For example, you might want to just say that it enforces that tables have certain Kudu-only properties, and leave it at that. 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? 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? 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 deletes/creates it, but do our tests actually use it? 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 kudu-client? Is maven smart enough to figure that out? 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? -- 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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 17 Oct 2017 21:21:41 +0000 Gerrit-HasComments: Yes
