> On Oct. 20, 2014, 9: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. > > Qian Xu wrote: > Added a TODO marker for that
the patch is in, if you rebase and have issues, let me know. > On Oct. 20, 2014, 9: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. > > Qian Xu wrote: > 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. makes sense if we are adding the FROM part in another patch > On Oct. 20, 2014, 9: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 > > Qian Xu wrote: > 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". please add javadocs on the kite conenctor features as well. > On Oct. 20, 2014, 9: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"; > > > > > > } > > Qian Xu wrote: > 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. can you post a link. like to read. > On Oct. 20, 2014, 9: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? > > Qian Xu wrote: > Not limited to HDFS. Here is an useful link > http://kitesdk.org/docs/current/guide/URIs/ thanks, this makes sense! > On Oct. 20, 2014, 9: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 ? > > Qian Xu wrote: > 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()`. we should guard the matched code, it needs to be defensive, how can we expect that everyine will return a new Schema, my first gut was to return null. can you file a issue for that? > On Oct. 20, 2014, 9:14 p.m., Veena Basavaraj wrote: > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/LinkConfiguration.java, > > line 24 > > <https://reviews.apache.org/r/26963/diff/1/?file=726939#file726939line24> > > > > can we please move thi class to a sdk? and resue it in all connectors? > > > > also nitpick please rename link to linkconfig. > > > > link has a very different meaning in sqoop please file a ticket for me, I can move it > On Oct. 20, 2014, 9: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 > > Qian Xu wrote: > Unfortunately, ConfigUtils supports String Map Integer Boolean and Enum > only. Kite's Format is actually a class. I am not sure I understand, I mean why not use the Kite sdk format class for avro, csv, parquet... if those are standard formats for the kite why doe we need another class here. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26963/#review57536 ----------------------------------------------------------- On Oct. 21, 2014, 12:07 a.m., Qian Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26963/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 12:07 a.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 > >
