----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25180/#review53139 -----------------------------------------------------------
common/src/main/java/org/apache/sqoop/job/etl/ExtractorContext.java <https://reviews.apache.org/r/25180/#comment92672> + 1 on this change, I was wondering why is it here. common/src/main/java/org/apache/sqoop/json/SubmissionBean.java <https://reviews.apache.org/r/25180/#comment92595> why not drop the connector in the name, I think it is obvious and it will be consistent with the actual string ( infact I had this exact same change in my RB in https://reviews.apache.org/r/25586/_ common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java <https://reviews.apache.org/r/25180/#comment92640> May be not in scope for this but, may be we can start using the objectmapper / jackson here so we dont incur more tech debt. common/src/main/java/org/apache/sqoop/model/MSubmission.java <https://reviews.apache.org/r/25180/#comment92673> I dont have full context on the design discussions on this topic Design: if not provided, can we fall back to what we do now? common/src/main/java/org/apache/sqoop/schema/Schema.java <https://reviews.apache.org/r/25180/#comment92675> Abe : why not wildcard? I rather prefer wildcard once there is more than 5-10 items? are we worried abt perf? common/src/main/java/org/apache/sqoop/schema/Schema.java <https://reviews.apache.org/r/25180/#comment92682> could be a immutable, since we should not allow modifying the schema objects once created. common/src/main/java/org/apache/sqoop/schema/Schema.java <https://reviews.apache.org/r/25180/#comment92676> minor: if we ae using a builder pattern, ( return this) can we drop the add? common/src/main/java/org/apache/sqoop/schema/SchemaError.java <https://reviews.apache.org/r/25180/#comment92677> can we start using from/to consistently in all our error messages as well? common/src/main/java/org/apache/sqoop/schema/SchemaMatchOption.java <https://reviews.apache.org/r/25180/#comment92678> would be nice to have a little more javadoc on what these types will do ( helps a newbie read the code ) common/src/main/java/org/apache/sqoop/schema/type/Column.java <https://reviews.apache.org/r/25180/#comment92679> knitpick: please format a space before true common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java <https://reviews.apache.org/r/25180/#comment92680> I would rename the test methods too, i have done this change in one other related RB I posted https://reviews.apache.org/r/25586/. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java <https://reviews.apache.org/r/25180/#comment92681> why not support an api that can take a set ( matchingOptions) would be nicer from an api perspective, I like the builder pattern we have connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties <https://reviews.apache.org/r/25180/#comment92683> why not keep the From and To? connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsInitializer.java <https://reviews.apache.org/r/25180/#comment92684> thanks for renaming to config...! I suggest we had a NullConfiguration class so it ca be used in places where is no configuration , but since I will be refactoring these soon, I will add support for it) connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java <https://reviews.apache.org/r/25180/#comment92685> is it a connector or the job? a user defined matching comes as part of the job ( if I understand correctly, if not I would like to chat more on this in person) connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java <https://reviews.apache.org/r/25180/#comment92686> can we create a static set to register the supported matchers ( and avoid the proliferation of ifs/else? ) connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java <https://reviews.apache.org/r/25180/#comment92687> can we please add TODO(Gwen): with a link to SQOOP ticket for the user defined support. connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java <https://reviews.apache.org/r/25180/#comment92688> I am not sure best is the right word here, since we have already defined a ordering ourselves in how we will match. connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java <https://reviews.apache.org/r/25180/#comment92698> please add a ticket/ name for this. I can create one if this is a hassle.:) if we dont have plans,lets remove TODO and add a NOTE connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java <https://reviews.apache.org/r/25180/#comment92695> can use a ternary one line. I kind of dont like if/else. connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java <https://reviews.apache.org/r/25180/#comment92697> a method like this probaby shoul be a utility class or util class that would help with other places we support this in sqoop for ser/deser connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/LocationMatcher.java <https://reviews.apache.org/r/25180/#comment92700> please add some java doc. Frankly the term location was not obvious to me at all, I had to dig deeper to understand it. connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/LocationMatcher.java <https://reviews.apache.org/r/25180/#comment92699> I would just do !(col.getNullable()). core/src/main/java/org/apache/sqoop/framework/JobManager.java <https://reviews.apache.org/r/25180/#comment92689> I have already cleaned this up in the https://reviews.apache.org/r/25586/ execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java <https://reviews.apache.org/r/25180/#comment92692> drop the connector in the SCHEMA_FROM_CONNECTOR shell/src/main/resources/shell-resource.properties <https://reviews.apache.org/r/25180/#comment92693> why not fro,/to in the labels too? - Veena Basavaraj On Sept. 3, 2014, 3:03 p.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25180/ > ----------------------------------------------------------- > > (Updated Sept. 3, 2014, 3:03 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-SQOOP-1367 > > > Description > ------- > > SQOOP-1378 - Sqoop2: From/To: Refactor schema > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/job/etl/ExtractorContext.java af03f0a > common/src/main/java/org/apache/sqoop/json/SchemaBean.java 468f7ee > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java > f6a9bbf > common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee > common/src/main/java/org/apache/sqoop/schema/Schema.java bbebab8 > common/src/main/java/org/apache/sqoop/schema/SchemaError.java 7c8c61e > common/src/main/java/org/apache/sqoop/schema/SchemaMatchOption.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/schema/type/Column.java 30c26a3 > common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e > > common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java > ab5bbd4 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java > 87e0862 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java > 73a49b1 > > connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties > a1302c0 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java > 2b1dec2 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > 883636c > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorError.java > 8a095d2 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsInitializer.java > d2d12a8 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > 6ed4087 > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > 1e8ab52 > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java > 91b594e > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java > PRE-CREATION > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/LocationMatcher.java > PRE-CREATION > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/NameMatcher.java > PRE-CREATION > > connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java > df6d30f > core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java > 92414d8 > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java > 1d60ba3 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java > e457cff > shell/src/main/java/org/apache/sqoop/shell/core/Constants.java efabc46 > shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java > 6dbd870 > shell/src/main/resources/shell-resource.properties 73a19e8 > > submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java > bfa6958 > > Diff: https://reviews.apache.org/r/25180/diff/ > > > Testing > ------- > > Added unit tests and did manual testing: > Hdfs->MySQL > MySQL->Hdfs > MySQL->MySQL (name based matching) > > > Thanks, > > Gwen Shapira > >
