You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Don Bosco Durai <bo...@apache.org> on 2017/01/02 17:58:16 UTC
Re: Review Request 55008: Currently the ranger-admin will be aborted
when
the exception occured. We should get the default value instead of aborted.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55008/#review160334
-----------------------------------------------------------
embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java (lines 312 - 313)
<https://reviews.apache.org/r/55008/#comment231489>
I agree with Alok, blocks without braces are like disastor waiting to happen. It would be good to enclose within brackets
embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java (line 315)
<https://reviews.apache.org/r/55008/#comment231490>
For utility methods, I generally prefer to provide the exception as the second param to the logger. This provides the stack trace to identify who made the call with the bad values.
Similar feedback for the references in getLongConfig
embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java (line 433)
<https://reviews.apache.org/r/55008/#comment231491>
Same feedback as before, better to pass the exception as parameter, than just printing the message.
- Don Bosco Durai
On Dec. 23, 2016, 6:54 a.m., Qiang Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55008/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2016, 6:54 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Reddy, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1280
> https://issues.apache.org/jira/browse/RANGER-1280
>
>
> Repository: ranger
>
>
> Description
> -------
>
> I failed to run the ranger-admin command and couldn't find any error messages in log file after set ranger.service.http.connector.attrib.maxHeaderCount=aa(string value) in ranger-admin-site.xml file. I analyzed the source codes and found the reason.
> I found there are two questions after check the source codes in EmbeddedServer.java.
> 1. No exception pop out when called Integer.parseInt() and Long.parseLong() function. Once abnormal, the program aborted directly without log.
> 2. The catch captures anomaly without log in the loadConfig function. It only calls e.printStackTrace() function.
>
> Currently the program will be aborted when the exception occured. We should get the default value instead of aborted.
>
> We have tested our patch strictly.
>
>
> Diffs
> -----
>
> embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java 7ebba8a
>
> Diff: https://reviews.apache.org/r/55008/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qiang Zhang
>
>
Re: Review Request 55008: Currently the ranger-admin will be aborted
when
the exception occured. We should get the default value instead of aborted.
Posted by Qiang Zhang <zh...@zte.com.cn>.
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, lines 312-314
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line312>
> >
> > I agree with Alok, blocks without braces are like disastor waiting to happen. It would be good to enclose within brackets
>
> Qiang Zhang wrote:
> At the beginning of the getIntConfig I have set a default value for the ret.
> int ret = defaultValue;
> The following logic can be executed when the retStr is null.
> else
> ret = defaultValue;
> Since we have default the value for ret, so there is no need to execute the logic.
> So we don't need to enclose within brackets.
>
> Don Bosco Durai wrote:
> Hi Qiang, the logic is correct and also the syntax is valid. The feedback is just regarding the curly braces. Later on, if someone adds another line for the same if condition, but not put the braces, then it would have some unexpected consequences. Having the braces is just a good practice even for one line statements and is followed by many.
Ok. Thanks! You are right. I updated the code according to your opinion.
- Qiang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55008/#review160334
-----------------------------------------------------------
On Dec. 23, 2016, 6:54 a.m., Qiang Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55008/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2016, 6:54 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Reddy, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1280
> https://issues.apache.org/jira/browse/RANGER-1280
>
>
> Repository: ranger
>
>
> Description
> -------
>
> I failed to run the ranger-admin command and couldn't find any error messages in log file after set ranger.service.http.connector.attrib.maxHeaderCount=aa(string value) in ranger-admin-site.xml file. I analyzed the source codes and found the reason.
> I found there are two questions after check the source codes in EmbeddedServer.java.
> 1. No exception pop out when called Integer.parseInt() and Long.parseLong() function. Once abnormal, the program aborted directly without log.
> 2. The catch captures anomaly without log in the loadConfig function. It only calls e.printStackTrace() function.
>
> Currently the program will be aborted when the exception occured. We should get the default value instead of aborted.
>
> We have tested our patch strictly.
>
>
> Diffs
> -----
>
> embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java 7ebba8a
>
> Diff: https://reviews.apache.org/r/55008/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qiang Zhang
>
>
Re: Review Request 55008: Currently the ranger-admin will be aborted
when
the exception occured. We should get the default value instead of aborted.
Posted by Qiang Zhang <zh...@zte.com.cn>.
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, lines 312-314
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line312>
> >
> > I agree with Alok, blocks without braces are like disastor waiting to happen. It would be good to enclose within brackets
At the beginning of the getIntConfig I have set a default value for the ret.
int ret = defaultValue;
The following logic can be executed when the retStr is null.
else
ret = defaultValue;
Since we have default the value for ret, so there is no need to execute the logic.
So we don't need to enclose within brackets.
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, line 316
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line316>
> >
> > For utility methods, I generally prefer to provide the exception as the second param to the logger. This provides the stack trace to identify who made the call with the bad values.
> >
> > Similar feedback for the references in getLongConfig
Ok. Thanks. We will update the error message as below:
LOG.warning(retStr + " can't be parsed to int. Reason: " + err.toString());
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, line 434
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line434>
> >
> > Same feedback as before, better to pass the exception as parameter, than just printing the message.
Ok. Thanks. We will update the error message as below:
LOG.warning("Load configuration fail. Reason: " + e.toString() );
- Qiang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55008/#review160334
-----------------------------------------------------------
On Dec. 23, 2016, 6:54 a.m., Qiang Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55008/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2016, 6:54 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Reddy, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1280
> https://issues.apache.org/jira/browse/RANGER-1280
>
>
> Repository: ranger
>
>
> Description
> -------
>
> I failed to run the ranger-admin command and couldn't find any error messages in log file after set ranger.service.http.connector.attrib.maxHeaderCount=aa(string value) in ranger-admin-site.xml file. I analyzed the source codes and found the reason.
> I found there are two questions after check the source codes in EmbeddedServer.java.
> 1. No exception pop out when called Integer.parseInt() and Long.parseLong() function. Once abnormal, the program aborted directly without log.
> 2. The catch captures anomaly without log in the loadConfig function. It only calls e.printStackTrace() function.
>
> Currently the program will be aborted when the exception occured. We should get the default value instead of aborted.
>
> We have tested our patch strictly.
>
>
> Diffs
> -----
>
> embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java 7ebba8a
>
> Diff: https://reviews.apache.org/r/55008/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qiang Zhang
>
>
Re: Review Request 55008: Currently the ranger-admin will be aborted
when
the exception occured. We should get the default value instead of aborted.
Posted by Colm O hEigeartaigh <co...@apache.org>.
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, line 316
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line316>
> >
> > For utility methods, I generally prefer to provide the exception as the second param to the logger. This provides the stack trace to identify who made the call with the bad values.
> >
> > Similar feedback for the references in getLongConfig
>
> Qiang Zhang wrote:
> Ok. Thanks. We will update the error message as below:
> LOG.warning(retStr + " can't be parsed to int. Reason: " + err.toString());
I think it would be better instead to do:
LOG.log(Level.WARNING, retStr + " can't be parsed to int", err);
- Colm
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55008/#review160334
-----------------------------------------------------------
On Jan. 3, 2017, 7:18 a.m., Qiang Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55008/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 7:18 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Reddy, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1280
> https://issues.apache.org/jira/browse/RANGER-1280
>
>
> Repository: ranger
>
>
> Description
> -------
>
> I failed to run the ranger-admin command and couldn't find any error messages in log file after set ranger.service.http.connector.attrib.maxHeaderCount=aa(string value) in ranger-admin-site.xml file. I analyzed the source codes and found the reason.
> I found there are two questions after check the source codes in EmbeddedServer.java.
> 1. No exception pop out when called Integer.parseInt() and Long.parseLong() function. Once abnormal, the program aborted directly without log.
> 2. The catch captures anomaly without log in the loadConfig function. It only calls e.printStackTrace() function.
>
> Currently the program will be aborted when the exception occured. We should get the default value instead of aborted.
>
> We have tested our patch strictly.
>
>
> Diffs
> -----
>
> embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java 7ebba8a
>
> Diff: https://reviews.apache.org/r/55008/diff/
>
>
> Testing
> -------
>
>
> File Attachments
> ----------------
>
> 0001-RANGER-1280-update-patch-according-to-review.patch
> https://reviews.apache.org/media/uploaded/files/2017/01/03/f5977b4f-6251-4537-bb55-2c5068195445__0001-RANGER-1280-update-patch-according-to-review.patch
>
>
> Thanks,
>
> Qiang Zhang
>
>
Re: Review Request 55008: Currently the ranger-admin will be aborted
when
the exception occured. We should get the default value instead of aborted.
Posted by Don Bosco Durai <bo...@apache.org>.
> On Jan. 2, 2017, 5:58 p.m., Don Bosco Durai wrote:
> > embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java, lines 312-314
> > <https://reviews.apache.org/r/55008/diff/1/?file=1591529#file1591529line312>
> >
> > I agree with Alok, blocks without braces are like disastor waiting to happen. It would be good to enclose within brackets
>
> Qiang Zhang wrote:
> At the beginning of the getIntConfig I have set a default value for the ret.
> int ret = defaultValue;
> The following logic can be executed when the retStr is null.
> else
> ret = defaultValue;
> Since we have default the value for ret, so there is no need to execute the logic.
> So we don't need to enclose within brackets.
Hi Qiang, the logic is correct and also the syntax is valid. The feedback is just regarding the curly braces. Later on, if someone adds another line for the same if condition, but not put the braces, then it would have some unexpected consequences. Having the braces is just a good practice even for one line statements and is followed by many.
- Don Bosco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55008/#review160334
-----------------------------------------------------------
On Dec. 23, 2016, 6:54 a.m., Qiang Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55008/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2016, 6:54 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Reddy, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1280
> https://issues.apache.org/jira/browse/RANGER-1280
>
>
> Repository: ranger
>
>
> Description
> -------
>
> I failed to run the ranger-admin command and couldn't find any error messages in log file after set ranger.service.http.connector.attrib.maxHeaderCount=aa(string value) in ranger-admin-site.xml file. I analyzed the source codes and found the reason.
> I found there are two questions after check the source codes in EmbeddedServer.java.
> 1. No exception pop out when called Integer.parseInt() and Long.parseLong() function. Once abnormal, the program aborted directly without log.
> 2. The catch captures anomaly without log in the loadConfig function. It only calls e.printStackTrace() function.
>
> Currently the program will be aborted when the exception occured. We should get the default value instead of aborted.
>
> We have tested our patch strictly.
>
>
> Diffs
> -----
>
> embeddedwebserver/src/main/java/org/apache/ranger/server/tomcat/EmbeddedServer.java 7ebba8a
>
> Diff: https://reviews.apache.org/r/55008/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qiang Zhang
>
>