> On 十二月 21, 2018, 5:29 p.m., Arjun Mishra wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
> > Line 182 (original), 182 (patched)
> > <https://reviews.apache.org/r/69618/diff/1/?file=2115642#file2115642line182>
> >
> >     This does't do anything. Its a final and its value is false. Why will 
> > it be NULL?

Hi ,even Boolean is not primitive type .however Boolean can work well.


> On 十二月 21, 2018, 5:29 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
> > Line 55 (original), 55 (patched)
> > <https://reviews.apache.org/r/69618/diff/1/?file=2115643#file2115643line56>
> >
> >     We are only using a single instance of Gson, why not leave as is?

if “new Gson()” can not spend resources?I Modify it back


> On 十二月 21, 2018, 5:29 p.m., Arjun Mishra wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/69618/diff/1/?file=2115643#file2115643line63>
> >
> >     This should really be the only change. Thoughts?

I think Put close in finally will much better.


- hongtaq


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69618/#review211504
-----------------------------------------------------------


On 十二月 21, 2018, 10:16 a.m., hongtaq zhao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69618/
> -----------------------------------------------------------
> 
> (Updated 十二月 21, 2018, 10:16 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Anthony Young-Garner, kalyan kumar 
> kalvagadda, Li Pi, Na Li, Steve Moist, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2469
>     https://issues.apache.org/jira/browse/SENTRY-2469
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> I have fixed issues in RoleServlet.java. 
> 
> 
> RoleServlet at L62 needs to use either try resource or put it in the finally 
> block to avoid resource leak
> if (conf.getBoolean(ServerConfig.SENTRY_WEB_ENABLE, 
> ServerConfig.SENTRY_WEB_ENABLE_DEFAULT)) may return an NPE if conf.getBoolean 
> returns a nullBoolean (not the primitive type)
> RoleServlet at L45, use Preconditions.checkNotNull or Objects.requreNonNull 
> instead of assert
> RoleServlet at L55: String json = new Gson().toJson(roleMap);, it's better to 
> reuse the Gson instance.
> 
> 
> Diffs
> -----
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  4508361b 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/RolesServlet.java
>  9ed2bf31 
> 
> 
> Diff: https://reviews.apache.org/r/69618/diff/1/
> 
> 
> Testing
> -------
> 
> builded and tested it.
> 
> 
> Thanks,
> 
> hongtaq zhao
> 
>

Reply via email to