----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74221/#review225061 -----------------------------------------------------------
kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java Line 86 (original), 87 (patched) <https://reviews.apache.org/r/74221/#comment313869> Hi Madhan, This is not a code review comment, being new to KMS, just trying to understand the context of using these locks. Please correct me if I am wrong. As per new code, we are acquiring WriteLock on any CUD operation (like create, delete, rollover, flush) and ReadLock on redaing API like getKeyVersion() etc. There will be just one instance of "RangerkeyStoreprovider" per KMS process and we are taking one ReadWriteLock. That means, one CUD API operation will block the entire traffic even if more container threads are available. Although I understand that frequency of CUD APIs will be very less. If we are trying to fix the ConcurrentModificationException, then, simply making that DS thread safe should work like ConcurrentHashMap. I agree that in such cases, still chances of getting older values (like thread1 reads and thread2 was about to update that map/DS) exists. But we are reading the last updated value. If we are trying to ensure this part, then, what if GET and CUD API requests land on two different KMS instances? Can you please help with few cases where we are getting this concurrency issue. My only concern is, acquiring locks at top level service API may impact the number of request one KMS instance can serve. kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java Line 166 (original), 115 (patched) <https://reviews.apache.org/r/74221/#comment313870> Again same question, what race condition we are trying to avoid here ? - Vikas Kumar On Jan. 5, 2023, 2:17 a.m., Madhan Neethiraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74221/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2023, 2:17 a.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 > ----- > > > agents-common/src/main/java/org/apache/ranger/plugin/util/AutoClosableLock.java > PRE-CREATION > 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/6/ > > > Testing > ------- > > - added test script to call KMS APIs from multiple threads > - verified that stress_kms.py scripts completes successfully > > > Thanks, > > Madhan Neethiraj > >
