----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68286/#review207252 -----------------------------------------------------------
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java Lines 2821 (patched) <https://reviews.apache.org/r/68286/#comment290584> INCREMENT_POLICY_VERSION ==> POLICY_VERSION INCREMENT_TAG_VERSION ==> TAG_VERSION security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java Line 2864 (original), 2856 (patched) <https://reviews.apache.org/r/68286/#comment290582> Instead of creating multiple Runnable objects, one for each referringService, consider a single runnable that loops through referringServices. Also, consider adding VERSION_TYPE_INCREMENT_POLICY_AND_TAG_VERSION so that both can be updated in a single call. final VERION_TYPE versionToUpdate; if (filterForServicePlugin && isTagVersionUpdateNeeded) { versionToUpdate = VERSION_TYPE.INCREMENT_POLICY_AND_TAG_VERSION; } else { versionToUpdate = VERSION_TYPE.INCREMENT_POLICY_VERSION; } commitWork = new Runnable() { @Override public void run() { for(XXService referringService : referringServices) { persistVersionChange(daoMgr, referringService.getId(), versionToUpdate); } } }; transactionSynchronizationAdapter.executeOnTransactionCommit(commitWork); security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java Lines 171 (patched) <https://reviews.apache.org/r/68286/#comment290585> Consider avoiding afterCommit() method at the base class. Instead: - have afterCompletion(status) call this method - only when status == STATUS_COMMITTED - clear RUNNABLES_AFTER_COMMIT in afterCompletion(status). Currently this list might not be cleared if the transaction fails in commit. security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java Line 124 (original), 130 (patched) <https://reviews.apache.org/r/68286/#comment290583> Instead of creating multiple Runnable objects, one for each serviceVersionInfo, consider handling all updates within a single Runnable. - Madhan Neethiraj On Aug. 10, 2018, 12:58 a.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68286/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2018, 12:58 a.m.) > > > Review request for ranger, Madhan Neethiraj, Ramesh Mani, and Velmurugan > Periasamy. > > > Bugs: RANGER-2186 > https://issues.apache.org/jira/browse/RANGER-2186 > > > Repository: ranger > > > Description > ------- > > Policy updates to different policies within a service, when successful, > update the service's policy version. If the update transactions are > concurrent, and executed on different ranger-admin servers (in HA > configuration), then it is possible that policy-version of the transaction > that commits later overwrites policy-version of earlier transaction, > effectively losing track of the first change. > > If policy-version is updated after update to policy is committed, then the > window of such loss is greatly reduced. > > Similar considerations apply for tag updates. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/store/AbstractServiceStore.java > 69ded6dc8 > security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java > 0773616f9 > > security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java > 2a62fb408 > > security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java > e1003297a > security-admin/src/test/java/org/apache/ranger/biz/TestServiceDBStore.java > cb496ea8b > > > Diff: https://reviews.apache.org/r/68286/diff/1/ > > > Testing > ------- > > Passed all unit tests > > > Thanks, > > Abhay Kulkarni > >
