----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63920/#review193388 -----------------------------------------------------------
Fix it, then Ship it! Hi Szabi, I checked your patch and it seems to be good however I have some minor findings, please find them below. I ran unit and 3rd party tests with your patch succesfully. Thanks, Bogi src/java/org/apache/sqoop/SqoopOptions.java Lines 1087-1093 (original) <https://reviews.apache.org/r/63920/#comment271948> GenericJobStorage import is no longer used in this class after this change, please remove it too. src/test/com/cloudera/sqoop/metastore/TestMetastoreConfigurationParameters.java Lines 1 (patched) <https://reviews.apache.org/r/63920/#comment271949> Please add license info header to this new file too. src/test/com/cloudera/sqoop/metastore/TestMetastoreConfigurationParameters.java Lines 58-70 (patched) <https://reviews.apache.org/r/63920/#comment271952> Typo in test case and method names: "AutoAuto" - Boglarka Egyed On Dec. 8, 2017, 3:31 p.m., Szabolcs Vasas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63920/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2017, 3:31 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3255 > https://issues.apache.org/jira/browse/SQOOP-3255 > > > Repository: sqoop-trunk > > > Description > ------- > > This patch restores the following metastore behavior: > If --meta-connect parameter is present then Sqoop will try to connect to the > metastore database specified in this parameter value. It will use the > username and password specified in --meta-username and --meta-password > parameters if they are not present Sqoop will use empty username/password. If > the database in the connection string is not supported, then Sqoop will > default to HSQLDB metastore (See: > org.apache.sqoop.metastore.GenericJobStorage#canAccept). > > If --meta-connect parameter is not present Sqoop checks if > sqoop.metastore.client.enable.autoconnect configuartion parameter is set to > false. If the parameter is false then Sqoop throws an error since there are > no applicable metastore implementations. > The next step is to check sqoop.metastore.client.autoconnect.url > configuration parameter. In this parameter users can specify a JDBC > connection string to a metastore database (HSQLDB, MySQL, Oracle, PostgreSQL, > DB2 and SQL Server are supported) if it is not present Sqoop will create an > embedded HSQLDB metastore in the home directory of the OS user which executes > the sqoop command. The username and password can be specified in > sqoop.metastore.client.autoconnect.username (default value: SA) and > sqoop.metastore.client.autoconnect.password (default value is the empty > string). > > > SqoopOptions: > - It turned out that the default value initialisation did not work, because > the configuration parameters are not present in system properties just in the > configuration object > - getLocalAutoConnectString is moved to > AutoGenericJobStorage.getHomeDirFileConnectStr > > AutoGenericJobStorage: > - The class is restored version of AutoHsqldbStorage. > - it extends GenericJobStorage > > GenericJobStorage: > - the auto metastore parameters are moved to AutoGenericJobStorage > - driverClass field is removed since it can be derived from the > metastoreConnectStr > - canAccept is modified > > > JobTool: > - SUPPORTED_DRIVERS is moved to GenericJobStorage since JobTool just > delegates to job storages so the supported databases depend on the job > storage implementation. > - applyMetastoreOptions now copies the parameter values from the CommandLine > object to the SqoopObject instance. > - the storage descriptor map initialized in initializeStorageDescriptor now > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java > d5fdfba14f5a2a22ae41ce4627b4e9afd13d5d28 > src/java/org/apache/sqoop/metastore/AutoGenericJobStorage.java PRE-CREATION > src/java/org/apache/sqoop/metastore/GenericJobStorage.java > e4ffde210d6b93eec4cb4d3c5b20e388971bf564 > src/java/org/apache/sqoop/metastore/JobStorageFactory.java > 9a348d5181d20498dc89f68261302d65e041959a > src/java/org/apache/sqoop/tool/JobTool.java > 5b95c7d00643ed76d3e15799648ca40bda7f0b90 > src/test/com/cloudera/sqoop/TestIncrementalImport.java > 166792b516008f34f7d20f899a0910ed19155f6b > src/test/com/cloudera/sqoop/metastore/SavedJobsTestBase.java > 81789e76f2a5c9f85c38fed3e6fb7b791ed7299f > > src/test/com/cloudera/sqoop/metastore/TestMetastoreConfigurationParameters.java > PRE-CREATION > src/test/com/cloudera/sqoop/testutil/HsqldbTestServer.java > 8d0a30db8e3ab0b364f8ccf43e643269d4eb0df0 > src/test/org/apache/sqoop/metastore/TestAutoGenericJobStorageCanAccept.java > PRE-CREATION > src/test/org/apache/sqoop/metastore/TestGenericJobStorageCanAccept.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/63920/diff/2/ > > > Testing > ------- > > Executed unit and third party test suite, created new test cases. > > > Thanks, > > Szabolcs Vasas > >
