> On Nov. 16, 2015, 10:11 p.m., Abraham Fine wrote: > > common/src/main/java/org/apache/sqoop/json/JSONUtils.java, line 83 > > <https://reviews.apache.org/r/40335/diff/1/?file=1125872#file1125872line83> > > > > this null scares me. if i was to glance at this code i would see the > > containskey check at the top and then assume that we will always return a > > value. > > > > perhaps we should throw an exception when the value for a field is null > > (a different one than when the value for a field does not exist). > > Jarek Cecho wrote: > I see your point Abe - we could have lingering NullPointerExceptions down > the line. I'm somehow turned apart here - on one side we have bunch of places > where returning null is actually expected so always throwing exception is not > correct. We could add a parameter specifying what we should do on null > occasion, but I'm concerned that it would decrease readability of the code > (that is hard to read already). > > Would you be fine with keeping the current version of the patch?
sure. i look forward to the day we upgrade to java 8 and get optionals. - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40335/#review106744 ----------------------------------------------------------- On Nov. 15, 2015, 11:38 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40335/ > ----------------------------------------------------------- > > (Updated Nov. 15, 2015, 11:38 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2688 > https://issues.apache.org/jira/browse/SQOOP-2688 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > The patch looks quite huge, but it's really just changing a lot of accessors > to use the safe one. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/json/ConnectorBean.java 2509e3e > common/src/main/java/org/apache/sqoop/json/ConnectorsBean.java 88b71f5 > common/src/main/java/org/apache/sqoop/json/DriverBean.java 6f03be2 > common/src/main/java/org/apache/sqoop/json/JSONUtils.java ff8f9ea > common/src/main/java/org/apache/sqoop/json/JobBean.java df7a804 > common/src/main/java/org/apache/sqoop/json/JobsBean.java c62ab49 > common/src/main/java/org/apache/sqoop/json/LinkBean.java 99e7319 > common/src/main/java/org/apache/sqoop/json/LinksBean.java 6e4a906 > common/src/main/java/org/apache/sqoop/json/PrincipalBean.java e540b75 > common/src/main/java/org/apache/sqoop/json/PrincipalsBean.java a4d56dc > common/src/main/java/org/apache/sqoop/json/PrivilegeBean.java 6819063 > common/src/main/java/org/apache/sqoop/json/PrivilegesBean.java fbca54b > common/src/main/java/org/apache/sqoop/json/RoleBean.java e1f5783 > common/src/main/java/org/apache/sqoop/json/RolesBean.java da8fc1f > common/src/main/java/org/apache/sqoop/json/SubmissionBean.java c7d6958 > common/src/main/java/org/apache/sqoop/json/SubmissionsBean.java 019abf1 > common/src/main/java/org/apache/sqoop/json/ValidationResultBean.java > 7dfd9fc > > common/src/main/java/org/apache/sqoop/json/util/ConfigInputSerialization.java > bd4f5c4 > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java > 1fa5743 > common/src/main/java/org/apache/sqoop/json/util/SerializationError.java > d5cdb31 > common/src/test/java/org/apache/sqoop/json/TestJSONUtils.java 67ab412 > > Diff: https://reviews.apache.org/r/40335/diff/ > > > Testing > ------- > > I've run only unit tests, let's see how the precommit hook will like the > patch. > > > Thanks, > > Jarek Cecho > >
