----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65920/#review198707 -----------------------------------------------------------
And one question, why there is a uploadTagsWithCredUnSync ? It's not clear from the code, why the play with sync/unsync was needed ? tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 63 (patched) <https://reviews.apache.org/r/65920/#comment278936> RANGERADMINSESSIONID is not a final static constant, 'sessionId' could be a perfectly valid name tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 67 (patched) <https://reviews.apache.org/r/65920/#comment278937> cookieList tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 267 (patched) <https://reviews.apache.org/r/65920/#comment278930> Instead of relying on an external projects toString method, could you just use response.getLocation() ? tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 286 (patched) <https://reviews.apache.org/r/65920/#comment278931> The 3 if all contains (response != null) - this could be moved to an outer if : if (response != null) { if (a) ... if (b) ... if (c) ... } Or isn't it true that response is not null? tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 324 (patched) <https://reviews.apache.org/r/65920/#comment278932> Assigning value twice to CookieList - null, and response.getCookies(). The first is not needed. CookieList is a variable, not a type, why the upper case ? tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 326 (patched) <https://reviews.apache.org/r/65920/#comment278933> Cookie should be lowercased tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 343 (patched) <https://reviews.apache.org/r/65920/#comment278934> response.toString() tagsync/src/main/java/org/apache/ranger/tagsync/sink/tagadmin/TagAdminRESTSink.java Lines 362 (patched) <https://reviews.apache.org/r/65920/#comment278935> response.toString() - Zsombor Gegesy On March 6, 2018, 11:16 a.m., Nikhil P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65920/ > ----------------------------------------------------------- > > (Updated March 6, 2018, 11:16 a.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/1/ > > > 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 > >
