Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-15 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/#review207382
---


Ship it!




Ship It!

- Madhan Neethiraj


On Aug. 15, 2018, 10:06 p.m., Abhay Kulkarni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68286/
> ---
> 
> (Updated Aug. 15, 2018, 10:06 p.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/RangerDaoManager.java 
> 2788a6109 
>   security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java 
> da89e041c 
>   
> 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/3/
> 
> 
> Testing
> ---
> 
> Passed all unit tests
> 
> 
> Thanks,
> 
> Abhay Kulkarni
> 
>

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-15 Thread Abhay Kulkarni 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/
---

(Updated Aug. 15, 2018, 10:06 p.m.)


Review request for ranger, Madhan Neethiraj, Ramesh Mani, and Velmurugan 
Periasamy.


Changes
---

Addressed review comment


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 (updated)
-

  
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/RangerDaoManager.java 
2788a6109 
  security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java 
da89e041c 
  
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/3/

Changes: https://reviews.apache.org/r/68286/diff/2-3/


Testing
---

Passed all unit tests


Thanks,

Abhay Kulkarni

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-15 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/#review207336
---


Fix it, then Ship it!





security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java
Line 93 (original), 122 (patched)


Instead of calling runRunnables() twice, for RUNNABLES_AFTER_COMMIT & 
RUNNABLES, consider creatig a single list and call once. It can avoid one 
additional transaction.


- Madhan Neethiraj


On Aug. 15, 2018, 4:23 p.m., Abhay Kulkarni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68286/
> ---
> 
> (Updated Aug. 15, 2018, 4:23 p.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/RangerDaoManager.java 
> 2788a6109 
>   security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java 
> da89e041c 
>   
> 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/2/
> 
> 
> Testing
> ---
> 
> Passed all unit tests
> 
> 
> Thanks,
> 
> Abhay Kulkarni
> 
>

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-15 Thread Abhay Kulkarni 


> On Aug. 14, 2018, 8:31 p.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/db/XXServiceVersionInfoDao.java
> > Line 124 (original), 130 (patched)
> > 
> >
> > Instead of creating multiple Runnable objects, one for each 
> > serviceVersionInfo, consider handling all updates within a single Runnable.

All post-commit/post-completion tasks are run in a single transaction. 
Therefore, overhead of running multiple Runnable tasks is not significantly 
higher than running one consolidated task. However, the complexity of 
consolidation code (in all conditions) does not justify potential savings of 
having one consolidated task.


- Abhay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/#review207252
---


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

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-15 Thread Abhay Kulkarni 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/
---

(Updated Aug. 15, 2018, 4:23 p.m.)


Review request for ranger, Madhan Neethiraj, Ramesh Mani, and Velmurugan 
Periasamy.


Changes
---

Addressed review comments


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 (updated)
-

  
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/RangerDaoManager.java 
2788a6109 
  security-admin/src/main/java/org/apache/ranger/db/RangerDaoManagerBase.java 
da89e041c 
  
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/2/

Changes: https://reviews.apache.org/r/68286/diff/1-2/


Testing
---

Passed all unit tests


Thanks,

Abhay Kulkarni

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-14 Thread Madhan Neethiraj 

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


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)


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)


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)


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

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-10 Thread Abhay Kulkarni 


> On Aug. 10, 2018, 11:58 a.m., Zsombor Gegesy wrote:
> > security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java
> > Lines 122 (patched)
> > 
> >
> > Why this complicated is machinery with the thread locals and lists and 
> > registration is needed? 
> > 
> > You can easily have the same functionality - running a 'Runnable' 
> > object after a transaction finish, with just like this:
> > 
> > ```java
> > public void executeOnTransactionCommit(Runnable runnable) { 
> > TransactionSynchronizationManager.registerSynchronization(new 
> > TransactionSynchronizationAdapter() {
> > public void afterCommit() {
> >runnable.run();
> > }
> >   }
> > }
> > ```

The main purpose of the 'machinery' is to group several tasks together and 
accomplish them as one atomic unit after transaction is committed. The tasks 
themselves have only one thing in common; that they need to be all done or all 
rolled back. Such atomicity cannot be achieved with the shown code fragment.


- Abhay


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/#review207072
---


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

Re: Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-10 Thread Zsombor Gegesy 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/#review207072
---




security-admin/src/main/java/org/apache/ranger/common/db/RangerTransactionSynchronizationAdapter.java
Lines 122 (patched)


Why this complicated is machinery with the thread locals and lists and 
registration is needed? 

You can easily have the same functionality - running a 'Runnable' object 
after a transaction finish, with just like this:

```java
public void executeOnTransactionCommit(Runnable runnable) { 
TransactionSynchronizationManager.registerSynchronization(new 
TransactionSynchronizationAdapter() {
public void afterCommit() {
   runnable.run();
}
  }
}
```


- Zsombor Gegesy


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

Review Request 68286: RANGER-2186: Increment service-specific policy and tag versions after update transaction is committed

2018-08-09 Thread Abhay Kulkarni 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68286/
---

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