Skip to content

Conversation

@devmadhuu
Copy link
Contributor

What changes were proposed in this pull request?

Intermittent Delete root failed where it is observed that listStatus for FSO returns deleted files. This is causing flaky for some test cases.
Problem: listStatus called before flush of double buffer can return deleted key / directory present in cache. And also old information from db.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9041

How was this patch tested?

This patch is tested by executing existing flaky test cases which is now running fine.

deveshsingh and others added 27 commits August 10, 2023 09:22
@errose28 errose28 changed the title Hdds 9041 - Intermittent Delete root failed HDDS-9041. Intermittent Delete root failed. Aug 21, 2023
@devmadhuu devmadhuu marked this pull request as ready for review August 22, 2023 04:47
@devmadhuu
Copy link
Contributor Author

@sumitagrawl - Pls review.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devmadhuu Thanks for working over this, LGTM

HeapEntry entry = heapIterator.next();
OzoneFileStatus status = entry.getStatus(prefixKey,
scmBlockSize, volumeName, bucketName, replication);
map.put(entry.key, status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is done with the assumption that the items from the cache iterator always come before those from the rockdb iterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @duongkame as per MinHeapIterator implementation.

}

return map.values();
return map.values().stream().filter(e -> e != null).collect(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the status ever be null? If it can be, will putting the null check before putting it in the map a better idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duongkame thanks for review. Earlier the null value (means key deleted) from CacheIter was not added in map and was getting lost, so we should allow that and when keyInfo gets null and then getStatus , we'll return null. This was needed to allow deleted key in cache.

Copy link
Contributor

@duongkame duongkame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, @devmadhuu . LGTM.

@sumitagrawl sumitagrawl merged commit 011de37 into apache:master Aug 24, 2023
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Oct 24, 2023
(cherry picked from commit 011de37)
Change-Id: Ifad4eff7e6089467f0249c3435b36a8ff78eeffa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants