----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37539/#review95614 -----------------------------------------------------------
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? common/src/main/java/org/apache/sqoop/common/MapContext.java (line 164) <https://reviews.apache.org/r/37539/#comment150692> 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 common/src/main/java/org/apache/sqoop/common/MapContext.java (lines 200 - 202) <https://reviews.apache.org/r/37539/#comment150694> 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? Jarcec - Jarek Cecho 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 > >
