----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29090/#review65564 -----------------------------------------------------------
Looks pretty close overall. Just a few comments. connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnectorError.java <https://reviews.apache.org/r/29090/#comment108791> I think this should be "Error occurred while creating partitions" connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetExecutor.java <https://reviews.apache.org/r/29090/#comment108792> @VisibleForTesting thing should be package-private instead of protected, right? Protected leaks the method to children outside of this package, while package-private only allows access from within this package. connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetPartitioner.java <https://reviews.apache.org/r/29090/#comment108889> This is right that it isn't any more efficient to partition an un-partitioned dataset. But a partitioned dataset could return partitions. We can implement it later. connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetPartitioner.java <https://reviews.apache.org/r/29090/#comment108884> Why is this wrapped in a try/catch? Exception is always too general to catch. If there are no checked exceptions but there is still fear of failure, it should catch RuntimeException. connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteExtractor.java <https://reviews.apache.org/r/29090/#comment108886> Also should be package-private? connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteFromInitializer.java <https://reviews.apache.org/r/29090/#comment108890> Why doesn't this call `Datasets.exists` directly? connector/connector-kite/src/main/resources/kite-connector-config.properties <https://reviews.apache.org/r/29090/#comment108794> Without "store", this no longer makes sense. I would change it to "Dataset URI" because that could be used for both from and to URIs. - Ryan Blue On Dec. 16, 2014, 12:58 a.m., Qian Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29090/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2014, 12:58 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1647 > https://issues.apache.org/jira/browse/SQOOP-1647 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > This JIRA proposes to support reading data from an HDFS dataset. > > Here are two notes: > 1. Due to a review board warning, I moved new code from KiteDataTypeUtil.java > to AvroDataTypeUtil. > 2. Kite SDK seems have limitation of opening multiple record reader instances > by Sqoop 2's demand. This version supports only one > partition.https://groups.google.com/a/cloudera.org/forum/#!mydiscussions/cdk-dev/2NJUvh0WmoY > > > Diffs > ----- > > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnector.java > c864882abf352236a5142608c31b7180e5ae9101 > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteConnectorError.java > d67c8de3a65f83f50e94b54aff43a169aa4dbd2f > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetExecutor.java > 9432e4bf41dcd691f64546b18c7ede029b57e339 > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetPartition.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteDatasetPartitioner.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteExtractor.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteFromDestroyer.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteFromInitializer.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteLoader.java > 709fd94578eff5a982e0bcbd0aa2065958a7105f > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/KiteToDestroyer.java > 25912b496af9a71211177a5de3b45308c71389be > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/FromJobConfig.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/configuration/FromJobConfiguration.java > PRE-CREATION > > connector/connector-kite/src/main/java/org/apache/sqoop/connector/kite/util/AvroDataTypeUtil.java > PRE-CREATION > > connector/connector-kite/src/main/resources/kite-connector-config.properties > 27c77b468753cca291a62f76cb28fb1a12e2f051 > > connector/connector-kite/src/test/java/org/apache/sqoop/connector/kite/TestKiteExecutor.java > 5e4edc5084c526fd4f4da534e9a1f603417ce77d > > connector/connector-kite/src/test/java/org/apache/sqoop/connector/kite/TestKiteExtractor.java > PRE-CREATION > > connector/connector-kite/src/test/java/org/apache/sqoop/connector/kite/TestKiteFromInitializer.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/29090/diff/ > > > Testing > ------- > > Added new test cases. > > > Thanks, > > Qian Xu > >
