You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/08/09 12:35:43 UTC
[GitHub] [ozone] sky76093016 opened a new pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
sky76093016 opened a new pull request #2513:
URL: https://github.com/apache/ozone/pull/2513
## What changes were proposed in this pull request?
Avoid methods that generate NPE when calling `unit()`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/HDDS/issues/HDDS-5558
## How was this patch tested?
No need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#discussion_r685850425
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -50,6 +50,9 @@ public static long getTimeDurationHelper(String name, String vStr,
if (null == vUnit) {
LOG.warn("No unit for " + name + "(" + vStr + ") assuming " + unit);
vUnit = ParsedTimeDuration.unitFor(unit);
+ if(null == vUnit){
+ throw new NullPointerException("Unexpected unit" + unit);
Review comment:
I think `IllegalArgumentException` more closely matches the problem than `NullPointerException`.
```suggestion
throw new IllegalArgumentException("Unexpected unit: " + unit);
```
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -50,6 +50,9 @@ public static long getTimeDurationHelper(String name, String vStr,
if (null == vUnit) {
LOG.warn("No unit for " + name + "(" + vStr + ") assuming " + unit);
vUnit = ParsedTimeDuration.unitFor(unit);
+ if(null == vUnit){
Review comment:
Nit:
```suggestion
if (null == vUnit) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on a change in pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on a change in pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#discussion_r685876008
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -50,6 +50,9 @@ public static long getTimeDurationHelper(String name, String vStr,
if (null == vUnit) {
LOG.warn("No unit for " + name + "(" + vStr + ") assuming " + unit);
vUnit = ParsedTimeDuration.unitFor(unit);
+ if(null == vUnit){
+ throw new NullPointerException("Unexpected unit" + unit);
Review comment:
Thanks @adoroszlai for reviewing this too.
After query, `IllegalArgumentException` is indeed more suitable than `NullPointerException`.
I will replace it immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang merged pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
jojochuang merged pull request #2513:
URL: https://github.com/apache/ozone/pull/2513
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#discussion_r686081918
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -55,7 +56,7 @@ public static long getTimeDurationHelper(String name, String vStr,
}
long raw = Long.parseLong(vStr);
- long converted = unit.convert(raw, vUnit.unit());
+ long converted = unit.convert(raw, Objects.requireNonNull(vUnit).unit());
Review comment:
@jojochuang this is addressed in latest patch. Can you please take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#issuecomment-895769494
Thanks @jojochuang for reviewing this.I have solved it by throwing an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on a change in pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on a change in pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#discussion_r685867829
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -50,6 +50,9 @@ public static long getTimeDurationHelper(String name, String vStr,
if (null == vUnit) {
LOG.warn("No unit for " + name + "(" + vStr + ") assuming " + unit);
vUnit = ParsedTimeDuration.unitFor(unit);
+ if(null == vUnit){
Review comment:
Small details overlooked,thanks for reminding~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a change in pull request #2513: HDDS-5558. vUnit invocation unit() may produce NPE
Posted by GitBox <gi...@apache.org>.
jojochuang commented on a change in pull request #2513:
URL: https://github.com/apache/ozone/pull/2513#discussion_r685652295
##########
File path: hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/TimeDurationUtil.java
##########
@@ -55,7 +56,7 @@ public static long getTimeDurationHelper(String name, String vStr,
}
long raw = Long.parseLong(vStr);
- long converted = unit.convert(raw, vUnit.unit());
+ long converted = unit.convert(raw, Objects.requireNonNull(vUnit).unit());
Review comment:
if vUnit can be null, it would be better to check its nullity right after line 52, and throw an exception there (which can carry a more meaningful error message).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org