> On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java, line 55 > > <https://reviews.apache.org/r/25180/diff/2/?file=676054#file676054line55> > > > > 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/_
Got the version from your change after merging. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java, > > line 82 > > <https://reviews.apache.org/r/25180/diff/2/?file=676055#file676055line82> > > > > 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. Not in scope, but you are right :) > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/model/MSubmission.java, lines 109-114 > > <https://reviews.apache.org/r/25180/diff/2/?file=676056#file676056line109> > > > > 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? I prefer to keep it required (to signal intent - connectors must provide schemas). I highlighted that empty schemas are an option. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/Schema.java, line 24 > > <https://reviews.apache.org/r/25180/diff/2/?file=676057#file676057line24> > > > > Abe : why not wildcard? I rather prefer wildcard once there is more > > than 5-10 items? are we worried abt perf? Jarcec pet peeve. We accomodate those too :) > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/Schema.java, line 60 > > <https://reviews.apache.org/r/25180/diff/2/?file=676057#file676057line60> > > > > could be a immutable, since we should not allow modifying the schema > > objects once created. Agree in general, but in the latest change I completely dropped the set. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/Schema.java, line 112 > > <https://reviews.apache.org/r/25180/diff/2/?file=676057#file676057line112> > > > > minor: if we ae using a builder pattern, ( return this) can we drop the > > add? This function was removed. In the original, I wanted to match the syntax for "addColumn". > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/SchemaError.java, line 34 > > <https://reviews.apache.org/r/25180/diff/2/?file=676058#file676058line34> > > > > can we start using from/to consistently in all our error messages as > > well? I found "from and to schema don't match" somewhat unreadable (i.e. doesn't read like an actual sentence). I'll change in a later patch if we see that this confuses users. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/SchemaMatchOption.java, line 25 > > <https://reviews.apache.org/r/25180/diff/2/?file=676059#file676059line25> > > > > would be nice to have a little more javadoc on what these types will do > > ( helps a newbie read the code ) Added. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/main/java/org/apache/sqoop/schema/type/Column.java, line 43 > > <https://reviews.apache.org/r/25180/diff/2/?file=676060#file676060line43> > > > > knitpick: please format a space before true Done. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java, line 409 > > <https://reviews.apache.org/r/25180/diff/2/?file=676061#file676061line409> > > > > 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/. I got the new names from pulling your patch. As a rule, I try hard not to make modifications outside the scope of my patch, so if I'm not changing a function, I'm not renaming it either. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java, > > line 104 > > <https://reviews.apache.org/r/25180/diff/2/?file=676063#file676063line104> > > > > 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 Good idea. If we expose matching again, I'll add that. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-generic-jdbc/src/main/resources/generic-jdbc-connector-resources.properties, > > line 52 > > <https://reviews.apache.org/r/25180/diff/2/?file=676065#file676065line52> > > > > why not keep the From and To? I find this easier to understand. Will change if users are confused. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsInitializer.java, > > line 39 > > <https://reviews.apache.org/r/25180/diff/2/?file=676069#file676069line39> > > > > 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) Pulled your refactoring so we are good here. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java, > > line 201 > > <https://reviews.apache.org/r/25180/diff/2/?file=676071#file676071line201> > > > > 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) None of the above :) Its the IDF (technically part of the driver). Its used by both connectors. We are not sure where the user-defined matching will be placed yet, but I think it will be a job config. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java, > > line 400 > > <https://reviews.apache.org/r/25180/diff/2/?file=676071#file676071line400> > > > > can we create a static set to register the supported matchers ( and > > avoid the proliferation of ifs/else? ) I'm not sure we have a proliferation - this is the only place in the code where we decide on the matching logic. In any case, I greatly simplified the logic here, and since connectors no longer declare matching support, we don't need to register it. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java, > > line 411 > > <https://reviews.apache.org/r/25180/diff/2/?file=676071#file676071line411> > > > > can we please add TODO(Gwen): with a link to SQOOP ticket for the user > > defined support. done. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java, > > line 413 > > <https://reviews.apache.org/r/25180/diff/2/?file=676071#file676071line413> > > > > I am not sure best is the right word here, since we have already > > defined a ordering ourselves in how we will match. This part is gone. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java, > > line 25 > > <https://reviews.apache.org/r/25180/diff/2/?file=676073#file676073line25> > > > > 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 Replaced with NOTE since we don't have specific plans to add new IDFs. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java, > > line 46 > > <https://reviews.apache.org/r/25180/diff/2/?file=676073#file676073line46> > > > > can use a ternary one line. I kind of dont like if/else. I don't like ternaries :) > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/AbstractMatcher.java, > > line 54 > > <https://reviews.apache.org/r/25180/diff/2/?file=676073#file676073line54> > > > > 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 We don't use this in other places though. This is part of "getMatchingData", which is part of the matcher and is called when converting data between the two schemas. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/LocationMatcher.java, > > line 61 > > <https://reviews.apache.org/r/25180/diff/2/?file=676074#file676074line61> > > > > I would just do !(col.getNullable()). Sure :) > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/matcher/LocationMatcher.java, > > line 34 > > <https://reviews.apache.org/r/25180/diff/2/?file=676074#file676074line34> > > > > please add some java doc. Frankly the term location was not obvious to > > me at all, I had to dig deeper to understand it. Done. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 438 > > <https://reviews.apache.org/r/25180/diff/2/?file=676077#file676077line438> > > > > I have already cleaned this up in the > > https://reviews.apache.org/r/25586/ I pulled in your change. > On Sept. 12, 2014, 6:55 p.m., Veena Basavaraj wrote: > > shell/src/main/resources/shell-resource.properties, line 229 > > <https://reviews.apache.org/r/25180/diff/2/?file=676083#file676083line229> > > > > why not fro,/to in the labels too? I find this more readable as part of UI. I'll change it if users are confused. - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25180/#review53139 ----------------------------------------------------------- On Sept. 25, 2014, 12:52 a.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25180/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2014, 12:52 a.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 fd73890 > common/src/main/java/org/apache/sqoop/json/SchemaBean.java 468f7ee > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 9b1ae74 > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java > f6a9bbf > common/src/main/java/org/apache/sqoop/model/MSubmission.java ca21135 > 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/util/TestSchemaSerialization.java > ab5bbd4 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcFromInitializer.java > 9d0c178 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestExtractor.java > 776359a > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > 70833a0 > > 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 > 923f904 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > 7942d59 > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java > df5cb9c > > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java > 74b9518 > > 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/TestCSVIntermediateDataFormat.java > 8c83a71 > core/src/main/java/org/apache/sqoop/driver/JobManager.java 277c6be > docs/src/site/sphinx/Tools.rst ad72cd1 > execution/.DS_Store PRE-CREATION > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java > 2ed06a8 > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java > 6680f60 > > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java > eea0623 > shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java > 60acfb6 > shell/src/main/resources/shell-resource.properties b59bd81 > > submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java > fe92ac4 > > 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 > >
