You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2015/11/16 00:38:08 UTC
Review Request 40335: SQOOP-2688 Sqoop2: Provide utility method to
safely retrieve value from JSONObject
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40335/
-----------------------------------------------------------
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
Re: Review Request 40335: SQOOP-2688 Sqoop2: Provide utility method to
safely retrieve value from JSONObject
Posted by Jarek Cecho <ja...@apache.org>.
> 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).
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?
- Jarek
-----------------------------------------------------------
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
>
>
Re: Review Request 40335: SQOOP-2688 Sqoop2: Provide utility method to
safely retrieve value from JSONObject
Posted by Abraham Fine <ab...@cloudera.com>.
> 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
>
>
Re: Review Request 40335: SQOOP-2688 Sqoop2: Provide utility method to
safely retrieve value from JSONObject
Posted by Abraham Fine <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40335/#review106744
-----------------------------------------------------------
common/src/main/java/org/apache/sqoop/json/JSONUtils.java (line 83)
<https://reviews.apache.org/r/40335/#comment165503>
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).
- Abraham Fine
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
>
>
Re: Review Request 40335: SQOOP-2688 Sqoop2: Provide utility method to
safely retrieve value from JSONObject
Posted by Abraham Fine <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40335/#review106772
-----------------------------------------------------------
Ship it!
Ship It!
- Abraham Fine
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
>
>