> On April 16, 2018, 12:51 p.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/hive/HiveServer2Client.java > > Lines 88 (patched) > > <https://reviews.apache.org/r/66361/diff/5/?file=2002068#file2002068line88> > > > > Why not put this into a finally statement? No need to clean up after > > error?
The original implementation worked the same way so I did not want to change it, but the idea here is to not delete the files imported into HDFS when the Hive CREATE TABLE or the LOAD DATA INPATH command fails, so the user could manually load the data into Hive. > On April 16, 2018, 12:51 p.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > > Lines 136 (patched) > > <https://reviews.apache.org/r/66361/diff/5/?file=2002071#file2002071line136> > > > > I'm fine with abbreviations, however these might confuse those reading > > the code in the long run. It would make it easier for newbies to onboard > > and help less profficient users to understand what a command means if we > > weren't using them. > > > > So, we might want to follow the convention of not using abbreviations > > in the names of fields, constants, methods, etc. > > > > What is your opinion? This might be a good quesiton for the community > > as well, as we don't really have coding guidelines. > > Boglarka Egyed wrote: > I think this is a really good point however even the documentation uses > HS2 abbreviation thus it may not be that misleading as it seems to be: > https://cwiki.apache.org/confluence/display/Hive/HiveServer2+Overview Yes, usually I like to avoid abbreviations but I think in this case HS2 is a commonly known one and having --hiveserver2-url, --hiveserver2-user etc. as parameter names might make the Sqoop too long so I would keep it as is. > On April 16, 2018, 12:51 p.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > > Line 1854 (original), 1886-1904 (patched) > > <https://reviews.apache.org/r/66361/diff/5/?file=2002071#file2002071line1886> > > > > Nice validation! > > > > You could consider extracting two helper methods here, I believe: > > - validateAllOptionsAreSet(SqoopOption, List<String> optionkeys) throws > > InvalidOptionsException > > - validateOnlyOneisSet(SqoopOption, List<String> optionkeys) throws > > InvalidOptionsException > > > > or something similar... > > > > It would help validators reuse your nice error messages in the future! Yes, good idea but the problem is that the SqoopOptions fields cannot be easily found by the option key so these methods would be quite complex. I would keep this method as is for now. - Szabolcs ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66361/#review201189 ----------------------------------------------------------- On April 16, 2018, 9:12 a.m., Szabolcs Vasas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66361/ > ----------------------------------------------------------- > > (Updated April 16, 2018, 9:12 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3309 > https://issues.apache.org/jira/browse/SQOOP-3309 > > > Repository: sqoop-trunk > > > Description > ------- > > This JIRA covers the implementation of the client for HiveServer2 and its > integration into the classes which use HiveImport. > > - HiveClient interface is introduced with 2 implementation: > - HiveImport: this is the original implementation which uses HiveCLI > - HiveServer2Client: the new clients which connects to HS2 using JDBC > connection > - The common code is extracted to HiveCommon class > - HiveClient should be instantiated using HiveClientFactory which creates and > configures the right HiveClient based on the configuration in SqoopOptions > - HiveMiniCluster is introduced with a couple of helper classes to enable > end-to-end HS2 tests > - A couple of new options are added to SqoopOptions to be able to configure > the connection to HS2 > - Validation is implemented for these new options > > > Diffs > ----- > > build.xml 7f68b573c65a61150ca78d158084586c87775d84 > ivy.xml 6be4fa20fbbf1f303c69d86942b1874e18a14afc > src/docs/user/hive-args.txt 441f54e8e0cee63595937f4e1811abc2d89f9237 > src/docs/user/hive.txt 3dc8bb463d602d525fe5f2d07d52cb97efcbab7e > src/java/org/apache/sqoop/SqoopOptions.java > 651cebd69ee7e75d06c75945e3607c4fab7eb11c > src/java/org/apache/sqoop/hive/HiveClient.java PRE-CREATION > src/java/org/apache/sqoop/hive/HiveClientCommon.java PRE-CREATION > src/java/org/apache/sqoop/hive/HiveClientFactory.java PRE-CREATION > src/java/org/apache/sqoop/hive/HiveImport.java > c2729119d31f7e585f204f2d31b2051eea71b72b > src/java/org/apache/sqoop/hive/HiveServer2Client.java PRE-CREATION > src/java/org/apache/sqoop/hive/HiveServer2ConnectionFactory.java > PRE-CREATION > src/java/org/apache/sqoop/hive/TableDefWriter.java > b7a25b7809e0d50166966a77161dc8ff603fb2d2 > src/java/org/apache/sqoop/tool/BaseSqoopTool.java > b02e4fe7fda25c7f8171c7db17d15a7987459687 > src/java/org/apache/sqoop/tool/CreateHiveTableTool.java > d259566180369a55d490144e6f865e728f4f2e61 > src/java/org/apache/sqoop/tool/ImportAllTablesTool.java > 18f7a0af48d972d5186e9414475e080f1eb765f3 > src/java/org/apache/sqoop/tool/ImportTool.java > e9920058858653bec7407bf7992eb6445401e813 > src/test/org/apache/sqoop/hive/TestHiveClientFactory.java PRE-CREATION > src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java PRE-CREATION > src/test/org/apache/sqoop/hive/TestHiveServer2Client.java PRE-CREATION > src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java PRE-CREATION > src/test/org/apache/sqoop/hive/TestTableDefWriter.java > 8bdc3beb3677312ec0ee2e612616358bca4ca838 > src/test/org/apache/sqoop/hive/minicluster/AuthenticationConfiguration.java > PRE-CREATION > src/test/org/apache/sqoop/hive/minicluster/HiveMiniCluster.java > PRE-CREATION > > src/test/org/apache/sqoop/hive/minicluster/KerberosAuthenticationConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/hive/minicluster/NoAuthenticationConfiguration.java > PRE-CREATION > > src/test/org/apache/sqoop/hive/minicluster/PasswordAuthenticationConfiguration.java > PRE-CREATION > src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java PRE-CREATION > src/test/org/apache/sqoop/tool/TestHiveServer2OptionValidations.java > PRE-CREATION > src/test/org/apache/sqoop/tool/TestImportTool.java > 1c0cf4d863692f75bb8831e834fae47fc18b5df5 > > > Diff: https://reviews.apache.org/r/66361/diff/5/ > > > Testing > ------- > > Ran unit and third party tests suite. > > > Thanks, > > Szabolcs Vasas > >
