----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6962/#review11291 -----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24245> s/key/tablePropKey/? s/dryRun/isDryRun/g metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24234> Please give these counters descriptive names, e.g. tblLocCount and tblPropCount metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24237> Formatting. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24244> Are we sure the user is going to see this error message? If so, is the information actionable? Are enough details included in the log message so that the admin could actually go and fix the underlying problem? metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24248> Same deal here. I'm worried that the admin may not see this error, and that it doesn't include enough information to fix the problem. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24250> Seems like populating updateLocations makes sense even if this is not a dryRun. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24253> This looks hacky. We're losing information here because the return value doesn't differentiate between storage locations and table property fields. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24255> s/key/serdePropKey/ Raw types should not appear in parameter lists or on the LHS of expressions. s/HashMap/Map/ metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24256> formatting. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6962/#comment24257> s/HashMap/Map/ s/serdeKey/serdeParamKey/ s/tableProp/tablePropKey/ Using updateLocations as an output is not idiomatic Java. Why not make Map<String, String> the return type of this method and updateSerdeURI, updateMStorageDescriptorURI? metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24235> s/both/Both/ metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24236> Please fix the capitalization and space issues. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24261> Why is this public? metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24264> raw type in parameter list. I haven't flagged all of the places where this problem occurs. Please find and replace. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24263> raw type on LHS. metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24267> Please add bad URI records for the different fields and verify that the metatool takes a best-effort approach to completing the update operation. metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaTool.java <https://reviews.apache.org/r/6962/#comment24266> Add a check to see if it still contains the oldLocation? - Carl Steinbach On Sept. 10, 2012, 11:24 p.m., Shreepadma Venugopalan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6962/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2012, 11:24 p.m.) > > > Review request for hive, Carl Steinbach and Carl Steinbach. > > > Description > ------- > > This patch adds an additional option to Hive Metatool that allows Metatool to > take in a serdeParamKey from the user. Avro Serde's schema URL key used to be > called schema.url in the past whereas its called avro.schema.url now. The > purpose of the patch is to make Metatool more generic than what it is today > so that its in a position to handle variations such as the one described > above. The new option looks as below, > > -serdeParamKey <serde_param_key>=<value> > > Note that the new Option -serdeParamKey is valid only with the > -updateLocation option. When the user attempts to use the serdeParamKey > option with other options, an error is raised and the usage is printed. > > If the user doesn't pass -serdeParamKey as part of the -updateLocation > option, Hive Metatool searches for records with both "avro.schema.url" and > "schema.url" keys and updates them. > > > This addresses bug HIVE-3343. > https://issues.apache.org/jira/browse/HIVE-3343 > > > Diffs > ----- > > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 251d4ba > metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java > a76594a > metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaTool.java > 5790ae9 > > Diff: https://reviews.apache.org/r/6962/diff/ > > > Testing > ------- > > HiveMetaTool has been tested to verify that it handles both avro.schema.url > and schema.url correctly. Existing test case in TestHiveMetaTool.java has > been modified to use AvroSerDe instead of LazySimpleSerDe. > > > Thanks, > > Shreepadma Venugopalan > >
