----------------------------------------------------------- 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 > >
