Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Sqoop >> mail # dev >> Review Request: SQOOP-864 Sqoop2: Introduce ETL context objects


+
Jarek Cecho 2013-02-05, 19:47
Copy link to this message
-
Re: Review Request: SQOOP-864 Sqoop2: Introduce ETL context objects

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9311/#review16206
-----------------------------------------------------------
Jarcec, I fully support this. I agree that this will help to avoid breaking the compatibility of public API in the future.

I just have one suggestion, which can be applied to several places. Please see my comments inline.
connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportLoader.java
<https://reviews.apache.org/r/9311/#comment34638>

    Can you add the "getString(...)" method to ActorContext so that we can shorten this to "context.getString(..)"?
    
    In ActorContext.getString(...), you can return "context.getContext().getString(...)".
    
    I think this is cleaner.

connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportExtractor.java
<https://reviews.apache.org/r/9311/#comment34639>

    The same as above.

connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportPartitioner.java
<https://reviews.apache.org/r/9311/#comment34640>

    The same as above.

execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsSequenceImportLoader.java
<https://reviews.apache.org/r/9311/#comment34641>

    The same as above.

execution/mapreduce/src/main/java/org/apache/sqoop/job/etl/HdfsTextImportLoader.java
<https://reviews.apache.org/r/9311/#comment34642>

    The same as above.
- Cheolsoo Park
On Feb. 5, 2013, 7:46 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9311/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 7:46 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> This patch seems to be quite enormous, but the logic is fairly simple. I've created family of Context classes - one parent ActorContext and than for each ETL separate child. My reasoning is that each ETL class needs different set of arguments and thus should have it's own Context class. Then I've went and fix all implementations of the ETL classes to conform with the changes.
>
> Each context class is wrapping all parameters in a generic and extensible way. The only parameters that are not part of the Context objects are configuration objects. I wanted to move them into Context classes as well, but that did not work well with the generics, so I kept them separate.
>
>
> This addresses bug SQOOP-864.
>     https://issues.apache.org/jira/browse/SQOOP-864
>
>
> Diffs
> -----
>
>   common/src/main/java/org/apache/sqoop/etl/io/DataReader.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/etl/io/DataWriter.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/ActorContext.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/DestroyerContext.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/ExtractorContext.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/InitializerContext.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/LoaderContext.java PRE-CREATION
>   common/src/main/java/org/apache/sqoop/job/etl/PartitionerContext.java PRE-CREATION
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportDestroyer.java 2d53bdd0715de86d92ef5286855c8805301caadd
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportInitializer.java 06fbc519ab46ea01087e93398292a749b70630e2
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcExportLoader.java 5f8e129d43f208a434527edfb771daf67bdf98af
>   connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcImportDestroyer.java f7043ea2bd53409514ac7eb5197f65a076f683b9
+
Jarek Cecho 2013-02-08, 21:29
+
Cheolsoo Park 2013-02-10, 20:38
+
Jarek Cecho 2013-02-10, 20:47
+
Cheolsoo Park 2013-02-10, 20:52