> On July 3, 2019, 5:49 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > > Lines 2065 (patched) > > <https://reviews.apache.org/r/71007/diff/1/?file=2153486#file2153486line2065> > > > > Is it likely that policy or service could be null? If so, consider > > putting all logic here under a check : > > > > if (service != null && policy != null).
I am not sure what should be behaviour if policy is null. We can either throw the exception or we can skip other statements of the method. Since this api is mainly for improving bulk delete we can avoid throwing the exception. Now, I have added the check you suggested only for policy and not for the service. I am not adding a check for service as I want to enable this method for a use case when a user want to delete policy in bulk but policies may belong to different services ; in that case if he can't pass a specific service then he can pass service object value as null and the actual service object will be picked from policy object. Please review the updated patch. > On July 3, 2019, 5:49 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > > Lines 3010 (patched) > > <https://reviews.apache.org/r/71007/diff/1/?file=2153486#file2153486line3011> > > > > Consider moving lines 3010-3013 to after line 3008. @Abhay : Please read the comments added before the code. public void deleteZonePolicies(Collection<String> serviceNames, Long zoneId) throws Exception { if (CollectionUtils.isNotEmpty(serviceNames)) { XXPolicyDao policyDao = daoMgr.getXXPolicy(); for (String serviceName : serviceNames) { RangerService service = getServiceByName(serviceName); //below statement will pull only policy IDs rather XXPolicy object List<Long> policyIds = policyDao.findPolicyIdsByServiceNameAndZoneId(serviceName, zoneId); List<RangerPolicy> rangerPolicyList=new ArrayList<RangerPolicy>(); if (CollectionUtils.isNotEmpty(policyIds)) { for (Long id : policyIds) { //from the policy id we need to get RangerPolicy policy object which shall be added first in the rangerPolicyList rangerPolicyList.add(getPolicy(id)); } } // run through rangerPolicyList and delete ranger policies. for (RangerPolicy rangerPolicy : rangerPolicyList) { deletePolicy(rangerPolicy, service); } //rangerPolicyList object is required for Trx logs and Policy history. createTrxLogsAndHistoryAfterDelete(rangerPolicyList, service); } } } > On July 3, 2019, 5:49 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java > > Line 111 (original) > > <https://reviews.apache.org/r/71007/diff/1/?file=2153487#file2153487line111> > > > > Why is this change needed? https://thoughts-on-java.org/common-hibernate-mistakes-cripple-performance/ (Mistake 7) https://thoughts-on-java.org/hibernate-tips-remove-entities-persistence-context/ https://en.wikibooks.org/wiki/Java_Persistence/Persisting#Example_flush I came across few articles(mentioned above) and thought that if we are not going to reuse those deleted object then we don't need to call flush() for them as flush() call will immediately try to send delete entity statements to the DB. Also if all the bulk policy delete is going to be in single transactions then probably JPA will keep all these objects in a cache and update entities cascading in the memory untill the transaction completes. In the first articles its mentioned that the flush() call forces JPA/Eclipselink to perform a dirty check on all managed entities and to create and execute SQL statements for all pending insert, update or delete operations. That slows down the application because it prevents JPA/Eclipselink from using several internal optimizations. > On July 3, 2019, 5:49 p.m., Abhay Kulkarni wrote: > > security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java > > Line 262 (original) > > <https://reviews.apache.org/r/71007/diff/1/?file=2153487#file2153487line262> > > > > Why is this change needed? https://thoughts-on-java.org/common-hibernate-mistakes-cripple-performance/ (Mistake 7) https://thoughts-on-java.org/hibernate-tips-remove-entities-persistence-context/ https://en.wikibooks.org/wiki/Java_Persistence/Persisting#Example_flush I came across few articles(mentioned above) and thought that if we are not going to reuse those deleted object then we don't need to call flush() for them as flush() call will immediately try to send delete entity statements to the DB. Also if all the bulk policy delete is going to be in single transactions then probably JPA will keep all these objects in a cache and update entities cascading in the memory untill the transaction completes. In the first articles its mentioned that the flush() call forces JPA/Eclipselink to perform a dirty check on all managed entities and to create and execute SQL statements for all pending insert, update or delete operations. That slows down the application because it prevents JPA/Eclipselink from using several internal optimizations. - Pradeep ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71007/#review216348 ----------------------------------------------------------- On July 4, 2019, 7:17 a.m., Pradeep Agrawal wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71007/ > ----------------------------------------------------------- > > (Updated July 4, 2019, 7:17 a.m.) > > > Review request for ranger, Ankita Sinha, bhavik patel, Gautam Borad, Abhay > Kulkarni, Madhan Neethiraj, Mehul Parikh, Nikhil P, Nitin Galave, Ramesh > Mani, Sailaja Polavarapu, and Velmurugan Periasamy. > > > Bugs: RANGER-2493 > https://issues.apache.org/jira/browse/RANGER-2493 > > > Repository: ranger > > > Description > ------- > > ** Problem Statement ** Ranger takes an extremely long time to override a > service with many policies, will crash ranger if admin JVM heap size is the 1 > GB default > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > bf50df3a8 > security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java > 51c0de56c > security-admin/src/main/java/org/apache/ranger/db/XXPolicyDao.java > baf6b6ea4 > security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java > 171d73bfa > security-admin/src/main/resources/META-INF/jpa_named_queries.xml eb0384b14 > security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java > dc845cf80 > security-admin/src/test/java/org/apache/ranger/rest/TestServiceREST.java > 9b9aa8377 > > > Diff: https://reviews.apache.org/r/71007/diff/2/ > > > Testing > ------- > > Tested 500 policy import json and unable to reproduce JVM issue. > > > Thanks, > > Pradeep Agrawal > >
