sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Qian Xu" <sx.a...@googlemail.com>
Subject Re: Review Request 26963: SQOOP-1588: TO-side of Kite Connector - Write data to HDFS
Date Tue, 21 Oct 2014 07:07:37 GMT


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java,
line 40
> > <https://reviews.apache.org/r/26963/diff/1/?file=726930#file726930line40>
> >
> >     will the initializer/destroyer be different for FROM? 
> >     
> >     What is the plan for FROM? If I understand correctly this will not have a From
is it?
> >     
> >     If that is the case, then why not call this KiteToConnector. Also please add
javadoc and add notes that is here, would be useful to read what this connector does and does
not support.
> >     
> >     
> >     Destination: HDFS
> >     File Format: Avro Parquet and CSV.
> >     Compression Codec: Use default
> >     Partitioner Strategy: Not supported
> >     Column Mapping: Not supported

KiteConnector will have both FROM and TO. FROM will be added in another JIRA. In this patch,
any calls of FROM will raise exception with text "Not implemented".


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java,
line 37
> > <https://reviews.apache.org/r/26963/diff/1/?file=726930#file726930line37>
> >
> >     can we not call it KiteToConnector.
> >     
> >     I saw this convention in Hbase and Hdfs.

KiteConnector is correct. A connector will have FROM and TO. A FROM has FromInitializer Partitioner
Extractor and a FromDestroyer. A TO has ToInitializer Loader and ToDestroyer.


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java,
line 66
> > <https://reviews.apache.org/r/26963/diff/1/?file=726930#file726930line66>
> >
> >     Instead of exception, we could encourage
> >     
> >     We should use the NullConfiguration class in this case, saying this connector
does not support FROM
> >     
> >     just a thought.

Good thought. From the name, I guess NullConfiguration means nothing to configure. Actually
FROM has something to configure. For short-term wait, throw an exception is IMHO acceptable.


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteLoader.java,
line 31
> > <https://reviews.apache.org/r/26963/diff/1/?file=726934#file726934line31>
> >
> >     Q: is not the target here assumed to be always hdfs? can this be used to write
to anything else?

Not limited to HDFS. Here is an useful link http://kitesdk.org/docs/current/guide/URIs/


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteToInitializer.java,
line 83
> > <https://reviews.apache.org/r/26963/diff/1/?file=726936#file726936line83>
> >
> >     why cant be we return null ?

Good point. I checked the code, somewhere else (e.g. Matcher) will check schema with `schema.isEmpty()`.
If it returns `null` here, any check schema methods should be changed to `schema == null ||
schema.isEmpty()`.


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConfigUpgrader.java,
line 35
> > <https://reviews.apache.org/r/26963/diff/1/?file=726929#file726929line35>
> >
> >     headsup: this will need to change once 1551 is committed. Jarcec is still reviewing
it.

Added a TODO marker for that


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java,
line 71
> > <https://reviews.apache.org/r/26963/diff/1/?file=726930#file726930line71>
> >
> >     does even default make sense here? We have a enum for direction and it is already
typed, so why will there ever be the the default case?
> >     
> >     public enum Direction {
> >       FROM,
> >       TO
> >     }

`default` here is used to suppress a compiler error, otherwise compiler will complain about
missing a `return` statement.


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnectorError.java,
line 31
> > <https://reviews.apache.org/r/26963/diff/1/?file=726931#file726931line31>
> >
> >     what does destination mean, can we just say TO ?

Text changed to "Dataset exists already"


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConstants.java,
line 28
> > <https://reviews.apache.org/r/26963/diff/1/?file=726932#file726932line28>
> >
> >     can we just make the constants a interface? It is pretty much a standard pattern
to use if we need to extend some common constants in a sub entitiy.
> >     
> >     
> >     why do we need constructor for a class that holds static final strings?
> >     
> >     Might be another ticket, but it is one line change and 2 lines to delete.
> >     
> >     public interface Constants {
> >     
> >       /**
> >        * All job related configuration is prefixed with this:
> >        * <tt>org.apache.sqoop.job.</tt>
> >        */
> >       public static final String PREFIX_CONFIG = "org.apache.sqoop.job.";
> >     
> >       public static final String JOB_ETL_NUMBER_PARTITIONS = PREFIX_CONFIG
> >           + "etl.number.partitions";
> >     
> >       public static final String JOB_ETL_FIELD_NAMES = PREFIX_CONFIG
> >           + "etl.field.names";
> >     
> >       public static final String JOB_ETL_OUTPUT_DIRECTORY = PREFIX_CONFIG
> >           + "etl.output.directory";
> >     
> >       public static final String JOB_ETL_INPUT_DIRECTORY = PREFIX_CONFIG
> >           + "etl.input.directory";
> >     
> >     
> >     }

Googled a bit, it is considered as bad practice so far. I'm neutral for that, because it really
simplifies code. Better open a new clean-up jira.


> On Oct. 21, 2014, 12:14 p.m., Veena Basavaraj wrote:
> > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/ToFormat.java,
line 21
> > <https://reviews.apache.org/r/26963/diff/1/?file=726940#file726940line21>
> >
> >     can we be more specific on supported formats for hdfs via kite sdk, since this
will evolve as kote sdk evolves with newer formats.
> >     
> >     Why cant we use a enum class from Kite itself if there is one

Unfortunately, ConfigUtils supports String Map Integer Boolean and Enum only. Kite's Format
is actually a class.


- Qian


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


On Oct. 21, 2014, 3:07 p.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26963/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2014, 3:07 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1588
>     https://issues.apache.org/jira/browse/SQOOP-1588
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> Create a basic Kite connector that can write data (i.e. from a jdbc connection) to HDFS.
> 
> The scope is defined as follows:
> * Destination: HDFS
> * File Format: Avro Parquet and CSV.
> * Compression Codec: Use default
> * Partitioner Strategy: Not supported
> * Column Mapping: Not supported
> 
> 
> Diffs
> -----
> 
>   connector/connector-kite/pom.xml PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConfigUpgrader.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnectorError.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConstants.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetExecutor.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteLoader.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteToDestroyer.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteToInitializer.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteValidator.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/LinkConfig.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/LinkConfiguration.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/ToFormat.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/ToJobConfig.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/ToJobConfiguration.java
PRE-CREATION 
>   connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/util/KiteDataTypeUtil.java
PRE-CREATION 
>   connector/connector-kite/src/main/resources/kite-connector-config.properties PRE-CREATION

>   connector/connector-kite/src/main/resources/sqoopconnector.properties PRE-CREATION

>   connector/connector-kite/src/test/java/org/apache/sqoop/connector/kite/TestKiteExecutor.java
PRE-CREATION 
>   connector/connector-kite/src/test/java/org/apache/sqoop/connector/kite/TestKiteLoader.java
PRE-CREATION 
>   connector/connector-kite/src/test/resources/log4j.properties PRE-CREATION 
>   connector/pom.xml e98a0fc 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java
b385926 
>   pom.xml f25a29f 
>   server/pom.xml 67baaa5 
>   test/pom.xml 7a80710 
> 
> Diff: https://reviews.apache.org/r/26963/diff/
> 
> 
> Testing
> -------
> 
> New unittests included. All passed.
> 
> 
> Thanks,
> 
> Qian Xu
> 
>


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