sadanand48 commented on code in PR #5244:
URL: https://github.com/apache/ozone/pull/5244#discussion_r1317464774
##########
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:
If the problem about cache flush after obtaining the iterator reference of
the table is valid (which is the race condition mentioned here in the PR) , I
can see this call as problematic.
Let's take example of the caller `createFakeDirIfShould` , it returns a fake
dir instance to getFileStatus call if the key exists and if intermediate paths
don't. Here it decides whether it should create a fake dir or not based on
whether a key beyond the prefix exists in the table. If it exists, we also
check if it's deleted based on isKeyDeleted check. Now if it is deleted, we
will find the value in cache and fake dir is not created and getfileStatus will
return null. But if there is a cache flush, then we won't find any entry in
cache and this check will return false and create a fake dir but the key itself
doesn't exist. This can cause getFileStatus to be flaky. If we are not fixing
this method, we should probably evaluate its usage and address problems it can
cause.
```java
if (key.startsWith(targetKey)) {
if (!Objects.equals(key, targetKey)
&& !isKeyDeleted(key, keyTable)) {
LOG.debug("Fake dir {} required for {}", targetKey, key);
return createDirectoryKey(keyValue.getValue(), dirKey);
}
```
--
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]
