You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Veena Basavaraj <vy...@gmail.com> on 2015/03/23 22:13:24 UTC
Review Request 32415: GenericJdbcConnector : ToJobConfiguration and
Loader enhancements for Incremental write ( INSERT or UPDATE)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/
-----------------------------------------------------------
Review request for Sqoop and Jarek Cecho.
Bugs: SQOOP-1810
https://issues.apache.org/jira/browse/SQOOP-1810
Repository: sqoop-sqoop2
Description
-------
see JIRA for details on tests
Diffs
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 5af34a5
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java ab1ac86
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 400c0f2
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestJSONIntermediateDataFormat.java 0d9a1cf
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalWriteTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/32415/diff/
Testing
-------
see JIRA for details on tests
Thanks,
Veena Basavaraj
Re: Review Request 32415: Patch for SQOOP-1810
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/#review92658
-----------------------------------------------------------
Thank you for working on this one Veena. General feedback:
1) Do you think it would make sense to rename the "incremental write mode" to just a "write mode"? I don't see any persisted status here, so it doesn't seem like an incremental import. We're merely choosing a way we'll push data to the database.
2) I quite a few unrelated changes (splitting long lines), could we remove them from the patch?
3) Can we add coverage in integration tests suite?
Jarcec
- Jarek Cecho
On March 25, 2015, 8:18 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32415/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 8:18 p.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Bugs: SQOOP-1810
> https://issues.apache.org/jira/browse/SQOOP-1810
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see JIRA for details on tests
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java a5cd715
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java 22b726e
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java c2515a5
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java 3a73691
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 17bac77
>
> Diff: https://reviews.apache.org/r/32415/diff/
>
>
> Testing
> -------
>
> see JIRA for details on tests
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 32415: Patch for SQOOP-1810
Posted by Veena Basavaraj <vy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/
-----------------------------------------------------------
(Updated March 25, 2015, 1:18 p.m.)
Review request for Sqoop and Jarek Cecho.
Bugs: SQOOP-1810
https://issues.apache.org/jira/browse/SQOOP-1810
Repository: sqoop-sqoop2
Description
-------
see JIRA for details on tests
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java a5cd715
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java 22b726e
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java c2515a5
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java 3a73691
connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 17bac77
Diff: https://reviews.apache.org/r/32415/diff/
Testing
-------
see JIRA for details on tests
Thanks,
Veena Basavaraj
Re: Review Request 32415: Patch for SQOOP-1810
Posted by Veena Basavaraj <vy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/
-----------------------------------------------------------
(Updated March 25, 2015, 7:36 a.m.)
Review request for Sqoop and Jarek Cecho.
Summary (updated)
-----------------
Patch for SQOOP-1810
Bugs: SQOOP-1810
https://issues.apache.org/jira/browse/SQOOP-1810
Repository: sqoop-sqoop2
Description
-------
see JIRA for details on tests
Diffs
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 5af34a5
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java ab1ac86
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 400c0f2
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
Diff: https://reviews.apache.org/r/32415/diff/
Testing
-------
see JIRA for details on tests
Thanks,
Veena Basavaraj
Re: Review Request 32415: GenericJdbcConnector : ToJobConfiguration
and Loader enhancements for Incremental write ( INSERT or UPDATE)
Posted by Veena Basavaraj <vy...@gmail.com>.
> On March 24, 2015, 9:05 a.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java, lines 17-20
> > <https://reviews.apache.org/r/32415/diff/2/?file=903464#file903464line17>
> >
> > It seems that the UPSERT is not implemented yet, so I would suggest to not expose such configuration option to the user and "comment it out" similarly as we have for the "DELETE" example.
> >
> > (unless you will add it in subsequent patch as mentioned on the JIRA).
yes I added a prelim patch to get a early feedback, but that plan is to add it, so a select will be done before to see if it exists and then do a update / insert
> On March 24, 2015, 9:05 a.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java, lines 151-154
> > <https://reviews.apache.org/r/32415/diff/2/?file=903462#file903462line151>
> >
> > I think that we should expose ability for user to specify an updateColumn in case that the table doesn't have a primary key. I know that it sounds unlikely, but I've seen a lot of users that actually had such environments and need to export data into them.
> >
> > Also I think that we should add support for composite primary keys / update columns (e.g. if we need to identify the update row based on multiple columns).
can you elabborate more?
is the idea to validate the updateByColumn, at the time config values are added?
adding composite seems like a good idea, might be a good enhancement, since at this point it is hard to simulate that use case in the current integration test to add more support for such tables.
So I new ticket to enhace functionality would be good.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/#review77562
-----------------------------------------------------------
On March 23, 2015, 2:15 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32415/
> -----------------------------------------------------------
>
> (Updated March 23, 2015, 2:15 p.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Bugs: SQOOP-1810
> https://issues.apache.org/jira/browse/SQOOP-1810
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see JIRA for details on tests
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 5af34a5
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java ab1ac86
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 400c0f2
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
>
> Diff: https://reviews.apache.org/r/32415/diff/
>
>
> Testing
> -------
>
> see JIRA for details on tests
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 32415: Patch for SQOOP-1810
Posted by Veena Basavaraj <vy...@gmail.com>.
> On March 24, 2015, 9:05 a.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java, lines 17-20
> > <https://reviews.apache.org/r/32415/diff/2/?file=903464#file903464line17>
> >
> > It seems that the UPSERT is not implemented yet, so I would suggest to not expose such configuration option to the user and "comment it out" similarly as we have for the "DELETE" example.
> >
> > (unless you will add it in subsequent patch as mentioned on the JIRA).
>
> Veena Basavaraj wrote:
> yes I added a prelim patch to get a early feedback, but that plan is to add it, so a select will be done before to see if it exists and then do a update / insert
after discussions in JIRA that the "merge" is not consistently implemented standard by the most prominent Dbs, dropped the upsert use case
> On March 24, 2015, 9:05 a.m., Jarek Cecho wrote:
> > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java, lines 151-154
> > <https://reviews.apache.org/r/32415/diff/2/?file=903462#file903462line151>
> >
> > I think that we should expose ability for user to specify an updateColumn in case that the table doesn't have a primary key. I know that it sounds unlikely, but I've seen a lot of users that actually had such environments and need to export data into them.
> >
> > Also I think that we should add support for composite primary keys / update columns (e.g. if we need to identify the update row based on multiple columns).
>
> Veena Basavaraj wrote:
> can you elabborate more?
>
> is the idea to validate the updateByColumn, at the time config values are added?
>
> adding composite seems like a good idea, might be a good enhancement, since at this point it is hard to simulate that use case in the current integration test to add more support for such tables.
>
> So I new ticket to enhace functionality would be good.
have renamed the type to UPDATE_BY_COLUN, so in future a new one UPDATE_BY_COMPOSITE key can be added.
I have create a new ticket for this feature. I am not sure what should be the format of such a compounf key, comma separated or a list? do provide your inputs on it.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/#review77562
-----------------------------------------------------------
On March 25, 2015, 1:18 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32415/
> -----------------------------------------------------------
>
> (Updated March 25, 2015, 1:18 p.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Bugs: SQOOP-1810
> https://issues.apache.org/jira/browse/SQOOP-1810
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see JIRA for details on tests
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java a5cd715
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java 22b726e
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java c2515a5
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestLoader.java 3a73691
> connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java 17bac77
>
> Diff: https://reviews.apache.org/r/32415/diff/
>
>
> Testing
> -------
>
> see JIRA for details on tests
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 32415: GenericJdbcConnector : ToJobConfiguration
and Loader enhancements for Incremental write ( INSERT or UPDATE)
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/#review77562
-----------------------------------------------------------
Thank you for the patch Veena. I have couple of comments:
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java
<https://reviews.apache.org/r/32415/#comment125703>
I think that we should expose ability for user to specify an updateColumn in case that the table doesn't have a primary key. I know that it sounds unlikely, but I've seen a lot of users that actually had such environments and need to export data into them.
Also I think that we should add support for composite primary keys / update columns (e.g. if we need to identify the update row based on multiple columns).
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java
<https://reviews.apache.org/r/32415/#comment125683>
Missing header file.
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java
<https://reviews.apache.org/r/32415/#comment125682>
I believe that USER_ONLY is the default value right? So we perhaps don't need to specify it explicitly?
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java
<https://reviews.apache.org/r/32415/#comment125684>
Missing header file
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java
<https://reviews.apache.org/r/32415/#comment125686>
It seems that the UPSERT is not implemented yet, so I would suggest to not expose such configuration option to the user and "comment it out" similarly as we have for the "DELETE" example.
(unless you will add it in subsequent patch as mentioned on the JIRA).
Jarcec
- Jarek Cecho
On March 23, 2015, 9:15 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32415/
> -----------------------------------------------------------
>
> (Updated March 23, 2015, 9:15 p.m.)
>
>
> Review request for Sqoop and Jarek Cecho.
>
>
> Bugs: SQOOP-1810
> https://issues.apache.org/jira/browse/SQOOP-1810
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see JIRA for details on tests
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 5af34a5
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java ab1ac86
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 400c0f2
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
> connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
> connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
>
> Diff: https://reviews.apache.org/r/32415/diff/
>
>
> Testing
> -------
>
> see JIRA for details on tests
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 32415: GenericJdbcConnector : ToJobConfiguration
and Loader enhancements for Incremental write ( INSERT or UPDATE)
Posted by Veena Basavaraj <vy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32415/
-----------------------------------------------------------
(Updated March 23, 2015, 2:15 p.m.)
Review request for Sqoop and Jarek Cecho.
Bugs: SQOOP-1810
https://issues.apache.org/jira/browse/SQOOP-1810
Repository: sqoop-sqoop2
Description
-------
see JIRA for details on tests
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java f18acbd
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExecutor.java 5af34a5
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcLoader.java ab1ac86
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java 400c0f2
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteConfig.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/IncrementalWriteMode.java PRE-CREATION
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfiguration.java fd5d54b
connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-config.properties 52bf631
Diff: https://reviews.apache.org/r/32415/diff/
Testing
-------
see JIRA for details on tests
Thanks,
Veena Basavaraj