sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Fine" <abef...@cloudera.com>
Subject Re: Review Request 40896: SQOOP-2719
Date Tue, 08 Dec 2015 18:21:08 GMT


> On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote:
> > It seems that we have a lot of various fixes inside one patch. I hesitate to push
this as a single change as we will loose track of why exactly we did those changes. Would
you mind splitting each individual fix to it's own JIRA describing what is the problem and
hence why we need to fix it?

my concern with splitting this into a bunch of seperate patches is that many of these changes
are interdependent in nonobvious ways (due to database idiosynchracies). I figured that it
would be easier to merge it in together so we can have a baseline for working across multiple
databases. if you feel strongly about it, i can change it.


> On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote:
> > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java,
lines 243-247
> > <https://reviews.apache.org/r/40896/diff/5/?file=1154208#file1154208line243>
> >
> >     Out of my curiosity - what database throws RuntimeException when dropping non
existing table? Shouldn't we catch that inside the dropTable() method? (VERIFY)

the purpose of dropping tables is to clean up the test environment. in other words, it is
not essential that we successfully drop the tables (as they may not actually exist). we get
this behavior by default when we run our tests against a mini cluster, but we need to do it
manually if we are running against a real cluster and a real database. 

but after looking at the droptable method, that is what we are doing already. so i can remove
my exception catching logic. 
```java
  public void dropTable(TableName tableName) {
    StringBuilder sb = new StringBuilder("DROP TABLE ");
    sb.append(getTableFragment(tableName));

    try {
      executeUpdate(sb.toString());
    } catch(RuntimeException e) {
      LOG.info("Ignoring exception: " + e);
    }
  }
```


> On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote:
> > common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java,
lines 366-367
> > <https://reviews.apache.org/r/40896/diff/5/?file=1154208#file1154208line366>
> >
> >     Will this generally work for all databases that you've mentioned?

this should work for all three databases. i would not feel comfortable doing this with production
code, but for test code i think that it is fine.


> On Dec. 5, 2015, 2:38 p.m., Jarek Cecho wrote:
> > common-test/src/main/java/org/apache/sqoop/common/test/db/OracleProvider.java, lines
67-75
> > <https://reviews.apache.org/r/40896/diff/5/?file=1154210#file1154210line67>
> >
> >     This seems weird to me - if we need to split something with a dot, then that
something is not a table name. It's a database and table name at the same time. I would much
rather fix that code to distinguish that difference then do this sneaky splitting.

you are right. i have no idea why i put this in.


- Abraham


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40896/#review109086
-----------------------------------------------------------


On Dec. 5, 2015, 6:22 a.m., Abraham Fine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40896/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2015, 6:22 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2719
>     https://issues.apache.org/jira/browse/SQOOP-2719
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Sqoop2: Ensure the GenericJDBCConnector integration tests work against MySQL, PostgreSQL,
and Oracle
> 
> 
> Diffs
> -----
> 
>   common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java f30d587

>   common-test/src/main/java/org/apache/sqoop/common/test/db/MySQLProvider.java 393904f

>   common-test/src/main/java/org/apache/sqoop/common/test/db/OracleProvider.java b5f3104

>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExtractor.java
edb2754 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java
fa26c14 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartition.java
65400ef 
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcPartitioner.java
2a42ed4 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java
3b52128 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestPartitioner.java
3a767ab 
>   docs/src/site/sphinx/user/connectors/Connector-GenericJDBC.rst 347547d 
>   test/src/main/java/org/apache/sqoop/test/data/Cities.java fbbd7ef 
>   test/src/main/java/org/apache/sqoop/test/data/DataSet.java 5e109b1 
>   test/src/main/java/org/apache/sqoop/test/data/ShortStories.java b84339e 
>   test/src/main/java/org/apache/sqoop/test/data/UbuntuReleases.java f8aeb38 
>   test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java 4c5d3a8

>   test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c843448 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/AppendModeTest.java
8c65898 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/FromHDFSToHDFSTest.java
c39c8d6 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/HdfsIncrementalReadTest.java
e6f6e0d 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/InformalJobNameExecuteTest.java
411b07e 
>   test/src/test/java/org/apache/sqoop/integration/connector/hdfs/OutputDirectoryTest.java
1790f96 
>   test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java
0e46bf3 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java
5053b56 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java
25cdb68 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java
686572a 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java
38ebb74 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/PartitionerTest.java
72728fe 
>   test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableStagedRDBMSTest.java
68dc65e 
>   test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java
6e78a13 
>   test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java
7b2aced 
>   test/src/test/java/org/apache/sqoop/integration/connectorloading/BlacklistedConnectorTest.java
6228b0d 
>   test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java cbf1e90

>   test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java
9e682bc 
> 
> Diff: https://reviews.apache.org/r/40896/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Abraham Fine
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message