> 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 > >
