----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6650/#review10848 -----------------------------------------------------------
metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23414> Will do. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23413> I'll remove HiveMetaTool from the logging information. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23415> Will do. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23416> Will remove. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23411> This logic checks if the location URI on disk matches the location URI that the user has specified for the old-nn-location. ObjectStore retrieves the locationURI from the metastore and needs to verify that the locationURI in metastore matches the URI that the user has passed in before updating the records in the metastore. I'm not sure why this logic should be in HiveMetaTool. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23417> This logic checks if the URI in the metastore record matches the NN location URI that the user has passed in. ObjectStore retrieves the metastore records and decides to update a given record based on whether the record's current location URI matches the URI that the user has passed in. This logic is an integral and central part of the update logic which belongs in the ObjectStore. I don't see how this logic can belong elsewhere. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23388> Will remove. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23418> The output of the dryRun i.e., the current location and the new location are returned as a map of string values that are printed to stdout by HiveMetaTool. I think most of the logic in this function belongs in the ObjectStore because it uses the JDO and DN to access and write to the metastore records. Based on the previous revisions, I understand that we want to encapsulate all JDO/DN logic within ObjectStore. I'll rename dryRun to isDryRun. I think we should do the update in a single transaction ie., the HA upgrade should be atomic. If for some reason the upgrade fails, I think its better to have all of the records in the same state. The admin may later choose to retry the update. If we allowed non-atomic or partial updates then we would end up in a state where some records point to the HA NN and some others don't. In such a scenario some tables and the underlying data would be inaccessible. On the other hand if we allowed only an atomic upgrade and if the upgrade of the metastore failed the admin may choose not to use a HA NN till the HA upgrade of the metastore can be successfully completed. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23419> I'll split this method into updates for DB, SerdeParam and StorageDescriptor. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23420> Will do. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23421> Will remove. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23389> This tab was added by eclipse when I split the line up. I'll try to remove this. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23391> I think we should either update all the metastore records or none. Updating some of the records and failing in between leaves the metastore in an inconsistent state with some records updated and others not. I think the metastore update should be atomic. Please see my earlier comment on this. metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java <https://reviews.apache.org/r/6650/#comment23392> While some of the logic such as some of the printing can be moved to the metatool, most of the logic in this method i.e., the logic that accesses the metastore through JDO should probably reside in objectstore. While executing some of that logic it becomes useful to output information e.g., the stage of processing we are in etc. I think its OK to use LOG.info to do some of that since the purpose of doing so is for debugging if there is a problem. Please not that some other ObjectStore methods do that as well. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23380> Unfortunately the arg name can't be split into three - I tried that and the only the last arg is printed. On googling I found out that this is a known problem with the commons-cli and helpformatter classes. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23381> I can move the ObjectStore constructor to the init method if you like. However, I'd like to explain why I've it in the main method. Initializing the object store is an expensive operation and I think we should not do it unless we want to use the options such as -updateLocation, -listFSRoot etc that actually need the ObjectStore. We don't have to initialize the ObjectStore for instance to print the help menu. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23382> Done metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23383> I'll print the help menu here. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23384> I thought it was informative to print this information. If you like I'll remove all System.out.println that prints the stage of the pipeline that the tool is in. Please confirm that's what you want. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23385> listFSRoot() will return null if the commit of the transaction fails. So if an error occurs during the commit of the JDO transaction during execution of the listFSRoot() operation, this error will be triggered. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23431> I tried to handle this using commons-cli but there doesn't seem to be a good way to do this. I don't see a way to add an option within another option i.e, there doesn't seem to be a way to support nested options. dryRun is an option that is valid only in the context of the updateLocation option. So I can't create a new Options object for dryRun. If you have a suggestion on how to do this through commons-cli I'll be happy to hear. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23386> I'll make this change. metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java <https://reviews.apache.org/r/6650/#comment23387> This was originally in the finally block. In a previous revision you asked me to move the try catch as close as possible to the code that could potentially raise an exception. So I moved try .. catch block as close to parser.parse as possible. - Shreepadma Venugopalan On Aug. 29, 2012, 3:24 a.m., Shreepadma Venugopalan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6650/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2012, 3:24 a.m.) > > > Review request for hive and Carl Steinbach. > > > Description > ------- > > This patch implement hive metatool which, > > * lets admins perform a HA upgrade by patching the location of the NN in > Hive's metastore > * allows JDOQL to be executed against the metastore. > > > This addresses bug HIVE-3056. > https://issues.apache.org/jira/browse/HIVE-3056 > > > Diffs > ----- > > bin/ext/metatool.sh PRE-CREATION > bin/metatool PRE-CREATION > build.xml 6712af9 > eclipse-templates/TestHiveMetaTool.launchtemplate PRE-CREATION > metastore/ivy.xml 3011d2f > metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 045b550 > metastore/src/java/org/apache/hadoop/hive/metastore/tools/HiveMetaTool.java > PRE-CREATION > metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaTool.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6650/diff/ > > > Testing > ------- > > A new JUnit test - TestHiveMetaTool - has been added to test the various > metatool options. > > > Thanks, > > Shreepadma Venugopalan > >
