> On Aug. 17, 2015, 6:57 p.m., Jarek Cecho wrote: > > Thanks for the patch Abe! I have couple of high level points: > > > > 1) ImmutableContext is an interface describing how we're expecting > > ImmutableContext implementations to behave. I believe that we should simply > > define the methods there immediately - I don't see much value in adding > > them later. > > > > 2) Are you also thinking about adding corresponding set* methods to > > MutableContext?
This is a great point. Perhaps I should just create a set of utilities instead that are statically importable. > On Aug. 17, 2015, 6:57 p.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/common/MapContext.java, line 164 > > <https://reviews.apache.org/r/37539/diff/1/?file=1042204#file1042204line164> > > > > I somehow feel that the default value for case doesn't make much sense. > > We are requiring default value only for get* methods that are returning > > primitive type (int, bool, long, ...). It seems a bit overkill for objects, > > but we already have that for getString() [1], so I guess up to you :) > > > > Links: > > 1: > > https://github.com/apache/sqoop/blob/sqoop2/common/src/main/java/org/apache/sqoop/common/ImmutableContext.java This will normally be null I guess. > On Aug. 17, 2015, 6:57 p.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/common/MapContext.java, lines 200-202 > > <https://reviews.apache.org/r/37539/diff/1/?file=1042204#file1042204line200> > > > > There is nothing "jar" specific about this method right? It's just > > about getting set of string for given key. Let's perhaps rename it as such? True! Changed to getArrayOfUniqueStrings! - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37539/#review95614 ----------------------------------------------------------- On Aug. 17, 2015, 6:30 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37539/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2015, 6:30 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2491 > https://issues.apache.org/jira/browse/SQOOP-2491 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > External jar handling is added to the MapContext only. If we need this in > ImmutableMapContext (higher up in the inheritance tree), we can move it later. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/common/MapContext.java fc722c0 > common/src/test/java/org/apache/sqoop/common/TestMapContext.java 2a27c0c > core/src/main/java/org/apache/sqoop/core/SqoopConfiguration.java 58c4c34 > > Diff: https://reviews.apache.org/r/37539/diff/ > > > Testing > ------- > > mvn test > > > Thanks, > > Abraham Elmahrek > >
