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




kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStore.java
Line 131 (original), 109 (patched)
<https://reviews.apache.org/r/74221/#comment313862>

    This constructor should further call the third contructor.
    
    this(daoManager, false, null)



kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStore.java
Lines 1110 (patched)
<https://reviews.apache.org/r/74221/#comment313863>

    Concept of  using ReeadWriteLock this way is really  good, this way it is 
getting automatically unlocked due to AutoCloseable  interface.
    
    But this is redundant in many files, at least the boilerplate code I have 
seen in three files.
    
    Can't we use this as wrapper over these locks, something like following 
(just an extension to the existing logic):
    
      class AutocloseableLocker<T extends Lock> implements AutoCloseable{
            
            private T lock;
            public AutocloseableLocker(T lock){
                this.lock = lock;
            }
    
            @Override
            public void close() throws Exception {
                lock.unlock();
            }
        }
        
        
     Make this as utility class and create the instance of  the same class 
whereever required and keep the ref of the instance in the required class. This 
way, we will have just one class for Read & Write and no need to keep these in 
all classes where such logic is required.


- Vikas Kumar


On Dec. 11, 2022, 11:47 p.m., Madhan Neethiraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/74221/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2022, 11:47 p.m.)
> 
> 
> Review request for ranger, Ankita Sinha, Don Bosco Durai, Kishor 
> Gollapalliwar, Abhay Kulkarni, Mehul Parikh, Monika Kachhadiya, Pradeep 
> Agrawal, Ramesh Mani, Siddhesh Phatak, Sailaja Polavarapu, Subhrat Chaudhary, 
> Vikas Kumar, and Velmurugan Periasamy.
> 
> 
> Bugs: RANGER-3989
>     https://issues.apache.org/jira/browse/RANGER-3989
> 
> 
> Repository: ranger
> 
> 
> Description
> -------
> 
> - addressed concurrency issues in dealing with simultaneous requests
> - JPA object loading improvements, to avoid cache overheads
> - code readability improvements
> 
> 
> Diffs
> -----
> 
>   kms/config/kms-webapp/kms-logback.xml 16e82a656 
>   
> kms/src/main/java/org/apache/hadoop/crypto/key/AzureKeyVaultClientAuthenticator.java
>  13897ac3e 
>   kms/src/main/java/org/apache/hadoop/crypto/key/RangerKMSDB.java 812e7dfc4 
>   kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStore.java 
> 8dc129069 
>   kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java 
> cb5739f61 
>   kms/src/main/java/org/apache/hadoop/crypto/key/RangerMasterKey.java 
> c37e98ee5 
>   kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java 
> c83382d64 
>   
> kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java
>  d9f1b5b8e 
>   kms/src/main/java/org/apache/ranger/kms/biz/RangerKMSStartUp.java aae722b39 
>   kms/src/main/java/org/apache/ranger/kms/dao/BaseDao.java f97154cb9 
>   kms/src/main/java/org/apache/ranger/kms/dao/DaoManager.java c28e78535 
>   kms/src/main/java/org/apache/ranger/kms/dao/RangerKMSDao.java cb64310c2 
>   kms/src/main/java/org/apache/ranger/kms/dao/RangerMasterKeyDao.java 
> 22edc9a13 
>   kms/src/main/resources/META-INF/kms_jpa_named_queries.xml 94d5fa67c 
>   ranger-tools/src/main/python/stress/stress_kms.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/74221/diff/4/
> 
> 
> Testing
> -------
> 
> - added test script to call KMS APIs from multiple threads
> - verified that stress_kms.py scripts completes successfully
> 
> 
> Thanks,
> 
> Madhan Neethiraj
> 
>

Reply via email to