devmadhuu commented on code in PR #5244:
URL: https://github.com/apache/ozone/pull/5244#discussion_r1317177251
##########
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:
> Thanks @devmadhuu for explaining, missed the part where you added the null
value. makes sense to avoid the isKeyDeleted check. We should check other
usages of `isKeyDeleted` if it causes similar issues elsewhere.
>
> Maybe the correct way to check isKeyDeleted would be to check the
existence of the key in table if no value is present in cache.
Thanks @sadanand48 , I have handled the check inside isKeyDeleted method to
check the existence of the key in table if no value is present in cache. Few
other methods are calling this method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1426,7 +1426,9 @@ private void listStatusFindKeyInTableCache(
}
OzoneFileStatus fileStatus = new OzoneFileStatus(
cacheOmKeyInfo, scmBlockSize, !OzoneFSUtils.isFile(cacheKey));
- cacheKeyMap.put(cacheKey, fileStatus);
+ cacheKeyMap.putIfAbsent(cacheKey, fileStatus);
+ } else if (cacheOmKeyInfo == null) {
Review Comment:
> ```java
> else if (cacheOmKeyInfo == null && && cacheKey.startsWith(startCacheKey)
> && cacheKey.compareTo(startCacheKey) >= 0) ) {
> ```
>
> I think we should also check the key prefix as cacheKeyMap is only
constructed for key with given prefix.
Thanks @sadanand48 . I have handled the suggested change.
--
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]
