Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11798 )
Change subject: [docs] add Hive Metastore integration ...................................................................... Patch Set 6: (24 comments) http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc File docs/hive_metastore.adoc: http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@38 PS5, Line 38: : ## Databases and Table Names > Are there column name restrictions that are worth mentioning? There is not name restriction for columns in the Hive Metastore: https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java#L367 http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@42 PS5, Line 42: A > nit: not sure if jekyll will get rid of this, but there's an extra space he Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@45 PS5, Line 45: the table name must indicate the > nit: since there's still no such thing as a database in Kudu, maybe "the ta Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@46 PS5, Line 46: of a Hive datab > nit: maybe call out, "...identifiers (i.e. the table name and database name Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@59 PS5, Line 59: applications that > nit: "applications that" Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@58 PS5, Line 58: By including databases as an implicit part : of the Kudu table name, existing applications that use Kudu tables can operate : on non-HMS-integrated and HMS-integrated table names with minimal or no changes. > Maybe as a note, do you think it's worth calling out the previous conventio Right, I think that belongs to the Impala integration doc. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@61 PS5, Line 61: : Kudu provides no additional tooling to create or drop Hive databases. Administrators : or users should use existing Hive tools such as the Beel > I was a little confused about why this sentence was here, so maybe reword i Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@68 PS5, Line 68: naming constraints. Namely, the > nit: maybe just "naming constraints"? Maybe the section should be called "N Yeah, I think "naming constraints" maybe more specific. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@70 PS5, Line 70: `). > nit: underscores Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@73 PS5, Line 73: table name identifiers > Is this referring to both table names and database names? The configuration Yeah, this is for both table names and database names. Unfortunately, this is not consistent with the configuration name, I will make it more clear here. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@78 PS5, Line 78: Kudu will prevent tables from being created when a table only differs : by case. Additionally, operations that open, alter, or drop tables are : case-insensitive for both the database and table name portion of the Kudu : table name > nit reword a bit: Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@83 PS5, Line 83: Enabling the Hive Metastore Integration : > There should be a step here that renames existing Kudu tables. Put renames table in the 'Upgrading Existing Tables' section. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@86 PS5, Line 86: in, as well as allo > But this isn't necessarily on the command line? I think this is alluding to Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@101 PS5, Line 101: </property> : ``` : : * After bui > This isn't either of the listed updates to the XML. What is this doing? Can Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@108 PS5, Line 108: > nit: backticks and call it an environment variable? i.e. "`HIVE_AUX_JARS_PA I think I will remove this recommendations since it might be incorrect once Hive makes any related changes (and it is hard for us to detect without following up closely). http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@116 PS5, Line 116: > For parity, "match hive.metastore.uris"? At least from here: https://www.cl Discussed with Andrew offline, since 'hive.metastore.uris' is only used by the Hive Metastore client instead of the Hive Metastore, it is ok to keep it this way. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@121 PS5, Line 121: conflicting tables before enabling the Hive Metastore integration. : > I think these sections should be reordered: I prefer the current order. Since step 3 'Upgrading Existing Tables' should happen after enabling the HMS integration. The thing missing is the prerequisite for enabling the HMS integration: i.e table names that only differ by case will prevent the master(s) to start. I will move that up. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@125 PS5, Line 125: a Kudu cluster with existing : tables, an additional process, as a first step, m > Is this implying that administrators need to add the tables themselves, or It is both. I will rephrase it to be more clear. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@136 PS5, Line 136: O(hao): add a section about external table support > nit reword a bit: "Kudu tables with names that conflict only by case with a It isn't necessary to have 'a table entry in the Hive Metastore'. So I removed that. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@140 PS5, Line 140: u provides the > nit: maybe "administrative" instead? also capitalize "tools" to match other Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@151 PS5, Line 151: e > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@150 PS5, Line 150: me certain : inconsistencies require using a Hive-specific shell such as Beelin > I'm guessing this tool must be run while Kudu is online, right? And so the Yeah, it has to be run while Kudu is online. And this tool is not the first step in the upgrade process. I will make it more clear in the upgrade section. http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@158 PS5, Line 158: osed fix before actually exec > nit: perhaps give an example? "for instance, by renaming a Kudu table"? Done http://gerrit.cloudera.org:8080/#/c/11798/5/docs/hive_metastore.adoc@159 PS5, Line 159: fix can help. > How can a fix fail? Will a failed fix actually change the state of a cluste Yeah, it is actually the later is what I meant here. -- To view, visit http://gerrit.cloudera.org:8080/11798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12939c8f2245450ad46898c2050451b090c7ea01 Gerrit-Change-Number: 11798 Gerrit-PatchSet: 6 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alex Rodoni <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 12 Dec 2018 01:55:15 +0000 Gerrit-HasComments: Yes
