> On Aug. 16, 2019, 9:50 p.m., Pradeep Agrawal wrote: > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > > Line 2054 (original), 2058 (patched) > > <https://reviews.apache.org/r/71217/diff/1/?file=2160907#file2160907line2058> > > > > There is already a deletePolicy(RangerPolicy policy, RangerService > > service) method there. Possibly you can use that by passing the service > > object value as null. > > Nikhil P wrote: > we are not adding a new method but changing existing method in order to > avoid extra DB call for get policy, so I feel its better to get rid of the > method with Id param which gives additional DB call to get policy which is > already present with the caller method of it.Also there would be no backward > compatability issue as this is not the REST layer method. > > Pradeep Agrawal wrote: > Won't a call to deletePolicy(RangerPolicy policy, RangerService service) > method will serve the purpose here ? if yes, then either you can keep this > method as is "deletePolicy(Long policyId)" or remove the implementation of > this method completly(to avoid pmd issues).
This delete method with policy & service parameter is used generally for bulk delete,i.e policy loop at the caller method,so this method does not throw an error if one of the policy is null while deleting. it seems to ignore that policy and proceed to next, hence this method wont throw exception to the user. whereas the current method with only policy parameter gets used for single policy delete and it seems to check and throw an exception if that policy is null, as this method has the REST level caller which deletes single policy in one REST call. do we plan to keep single policy delete method in ServiceDBStore for both buld and single policy delete, If yes then we might need to think about throwing an exception if policy does not exists/null.But I suppose we dont throw an exception in case of bulk delete. - Nikhil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71217/#review217241 ----------------------------------------------------------- On Aug. 19, 2019, 3:21 p.m., Nikhil P wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71217/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2019, 3:21 p.m.) > > > Review request for ranger, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, > Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, Sailaja Polavarapu, and > Velmurugan Periasamy. > > > Bugs: RANGER-2535 > https://issues.apache.org/jira/browse/RANGER-2535 > > > Repository: ranger > > > Description > ------- > > Good coding practices for storing and retrieving data history in ranger > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/store/ServiceStore.java > 2af5845e2 > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 113e727c4 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > e7b317265 > > security-admin/src/main/java/org/apache/ranger/service/RangerDataHistService.java > 7bd0681b1 > > security-admin/src/main/java/org/apache/ranger/service/RangerPolicyLabelHelper.java > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/service/RangerPolicyLabelsService.java > 1a1b56e4c > security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java > 2be371892 > > > Diff: https://reviews.apache.org/r/71217/diff/2/ > > > Testing > ------- > > 1.Tested If data is getting stored and retrieved properly in data history > table i.e. x_data_hist. > > > Thanks, > > Nikhil P > >
