devmadhuu commented on code in PR #5244:
URL: https://github.com/apache/ozone/pull/5244#discussion_r1316832592
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1540,8 +1542,14 @@ public List<OzoneFileStatus> listStatus(OmKeyArgs args,
boolean recursive,
countEntries = 0;
// Convert results in cacheKeyMap to List
for (OzoneFileStatus fileStatus : cacheKeyMap.values()) {
- // No need to check if a key is deleted or not here, this is handled
- // when adding entries to cacheKeyMap from DB.
+ // Here need to check if a key is deleted as cacheKeyMap will contain
+ // deleted entries as well. Adding deleted entries in cacheKeyMap is done
+ // as there is a possible race condition where table cache iterator is
+ // flushed already and isKeyDeleted check may not work as expected
Review Comment:
@sadanand48 Thanks for your review. Pls check the
[changes](https://github.com/apache/ozone/pull/5244/files#diff-bde0dade7dd5ddda419499f4f999d25d40fcec1412e0ce809c36ffd1be473f22R1430-R1431)
in commit where we do not want to skip deleted keys,
`getIteratorForKeyInTableCache` methods gets called by
`org.apache.hadoop.ozone.om.KeyManagerImpl#listStatus(org.apache.hadoop.ozone.om.helpers.OmKeyArgs,
boolean, java.lang.String, long, java.lang.String, boolean)` where we call
cacheIterator on keyTable and then populate cacheKeyMap in
`listStatusFindKeyInTableCache` with non-deleted keys and that populated
cacheKeyMap is being used in `findKeyInDbWithIterator` method for wrapping the
OmKeyInfo object in OzoneFileStatus by having a check if key is not deleted.
Now here `isKeyDeleted` was used to check if key is deleted or not. This code
can have a race condition where keyTable cacheValue may itself may be null as
none of these methods are synchronized as a whole. Below `omKeyInfoCacheValu
e` may be null.
```
CacheValue<OmKeyInfo> omKeyInfoCacheValue
= keyTable.getCacheValue(new CacheKey(key));
```
So in `listStatusFindKeyInTableCache` method itself when iterating cacheIter
, we populate cacheKeyMap with deleted keys also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
