----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67689/#review205234 -----------------------------------------------------------
Hi Dani, I had a look at your patch and it basically looks good to me, it applied cleanly on my system and all tests passed. My only concern is that we lose a bit of test coverage. Wouldn't it make sense to reimplement the testcase you deleted in a different way? As far as I can see, it was the only test for external hive tables... It might take some effort to do this though, I didn't have time to understand how it works exactly. One might be able to reuse the code in the TestHiveImport class. src/java/org/apache/sqoop/hive/HiveImport.java Line 330 (original) <https://reviews.apache.org/r/67689/#comment288167> Was this config effectively only used in testing and no longer needed? - Fero Szabo On June 21, 2018, 1:39 p.m., daniel voros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67689/ > ----------------------------------------------------------- > > (Updated June 21, 2018, 1:39 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3323 > https://issues.apache.org/jira/browse/SQOOP-3323 > > > Repository: sqoop-trunk > > > Description > ------- > > When doing Hive imports the old way (not via JDBC that was introduced in > SQOOP-3309) we're trying to use the CliDriver class from Hive and fall back > to the hive executable (a.k.a. Hive Cli) if that class is not found. > > Since CliDriver and the hive executable that's relying on it are deprecated > (see also HIVE-10511), we should switch to using beeline to talk to Hive. > With recent additions (e.g. HIVE-18963) this should be easier than before. > > As a first step we could switch to using hive executable. With HIVE-19728 it > will be possible (in Hive 3.1) to configure hive to actually run beeline when > using the hive executable. This way we could leave it to the user to decide > whether to use the deprecated cli or use beeline instead. > > > Diffs > ----- > > src/java/org/apache/sqoop/hive/HiveImport.java 5da00a74 > src/test/org/apache/sqoop/TestIncrementalImport.java 1ab98021 > src/test/org/apache/sqoop/TestSqoopJobDataPublisher.java b3579ac1 > src/test/org/apache/sqoop/hive/TestHiveImport.java 436f0e51 > > src/test/org/apache/sqoop/manager/postgresql/PostgresqlExternalTableImportTest.java > dd4cfb48 > > > Diff: https://reviews.apache.org/r/67689/diff/1/ > > > Testing > ------- > > run thirdparty and normal UTs, also tested on a cluster > > I'm removing PostgresqlExternalTableImportTest since it was relying on the > CliDriver path to do an actual Hive import. > > > Thanks, > > daniel voros > >
