> On Jan. 5, 2023, 1:54 p.m., Vikas Kumar wrote: > > kms/src/main/java/org/apache/hadoop/crypto/key/RangerKeyStoreProvider.java > > Line 86 (original), 87 (patched) > > <https://reviews.apache.org/r/74221/diff/6/?file=2273446#file2273446line90> > > > > 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.
Vikas - I suggest running stress_kms.py (included in this patch) to reproduce failures in KMS handling of multiple simultaneous callers. For example, consider execution of following REST API calls simultaneously: 1. KMS.createKey() 2. KMS.rolloverKey(...) 3. KMS.invalidateCache(...) Each of these methods update RangerKeyStoreProvider.cache and RangerKeyStore.keyEntries - either to add/remove/clear. Protecting calls to these object with use of ConcurrentHashMap doesn't address failures in application handling of changes performed in other threads. Hence the lock should be moved up to a higher level. - Madhan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74221/#review225061 ----------------------------------------------------------- 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 > >
