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




tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 69 (patched)
<https://reviews.apache.org/r/65920/#comment279282>

    - consider moving 'static' field '_LOCK' ahead of instance members (to line 
#60 above)
    - also, consider marking this field as 'final'



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Line 179 (original), 195 (patched)
<https://reviews.apache.org/r/65920/#comment279283>

    'webResource' is used only inside the 'else' block at line #198. Consider 
moving this line (#195) to #199.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 228 (patched)
<https://reviews.apache.org/r/65920/#comment279284>

    "isValidRangerCookie == true" ==> "isValidRangerCookie"



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 276 (patched)
<https://reviews.apache.org/r/65920/#comment279290>

    Given this entire method implementation is under this synchronized() block, 
it might be easier to mark this method as 'synchroized' (and get rid of '_LOCK' 
object)



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 320 (patched)
<https://reviews.apache.org/r/65920/#comment279285>

    Instead of initializing with 'null' here and assigning below, consider 
initializing as shown below:
    
      WebResource         webResource = 
createCachedWebResource(REST_URL_IMPORT_SERVICETAGS_RESOURCE);
      WebResource.Builder br          = 
webResource.getRequestBuilder().cookie(sessionId);
      ClientResponse      response    = 
br.accept(REST_MIME_TYPE_JSON).type(REST_MIME_TYPE_JSON).put(ClientResponse.class,
 tagRESTClient.toJson(serviceTags));



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 328 (patched)
<https://reviews.apache.org/r/65920/#comment279286>

    when would 'response' not contain REST_URL_IMPORT_SERVICETAGS_RESOURCE? If 
there is such a case, can it not be covered by testing for response.getStatus()?
    
    Please review and update other such usage in this file.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 367 (patched)
<https://reviews.apache.org/r/65920/#comment279287>

    It is not clean why this is a 'cachedWebResource'. Please review and rename 
appropriately.



tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
Lines 369 (patched)
<https://reviews.apache.org/r/65920/#comment279289>

    'tagRESTClient.getClient()' can return null, if tagRESTClient.resetClient() 
was called. Please review and update to handle this case.


- Madhan Neethiraj


On March 9, 2018, 4:35 p.m., Nikhil P wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65920/
> -----------------------------------------------------------
> 
> (Updated March 9, 2018, 4:35 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Don Bosco Durai, Gautam Borad, Abhay 
> Kulkarni, Madhan Neethiraj, Pradeep Agrawal, Ramesh Mani, Selvamohan 
> Neethiraj, Sailaja Polavarapu, and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-2010
>     https://issues.apache.org/jira/browse/RANGER-2010
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> Ranger Tagsync should use cookie based authentication for subsequent requests 
> to Ranger admin.
> 
> 
> Diffs
> -----
> 
>   
> agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTClient.java
>  0d94edc 
>   tagsync/src/main/java/org/apache/ranger/tagsync/process/TagSyncConfig.java 
> 697c7cc 
>   
> tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java
>  a1dc8f5 
>   tagsync/src/main/resources/ranger-tagsync-site.xml 3542ae2 
> 
> 
> Diff: https://reviews.apache.org/r/65920/diff/2/
> 
> 
> Testing
> -------
> 
> 1)verified if ranger tagsync works as expected.
> 2)verified if tagsync is not re-login for every notification if 
> "ranger.tagsync.cookie.enabled" is set as true.
> 
> 
> Thanks,
> 
> Nikhil P
> 
>

Reply via email to