Skip to content

HDDS-7941. Race condition in getFileStatus causes flaky testObjectStoreCreateWithO3fs #5252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sumitagrawl merged 15 commits into from
Sep 22, 2023
Merged

HDDS-7941. Race condition in getFileStatus causes flaky testObjectStoreCreateWithO3fs #5252

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2eb1f36
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 24, 2023
56462ad
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 25, 2023
9a06841
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 25, 2023
0cb0131
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 30, 2023
d2563bc
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 30, 2023
0fd54fc
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 30, 2023
4d61435
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Aug 30, 2023
9826f48
Merge remote-tracking branch 'origin/master' into HDDS-7941
Sep 6, 2023
b0ce191
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 6, 2023
4056b12
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 6, 2023
782b197
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 6, 2023
23a598a
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 6, 2023
08982cb
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 7, 2023
667d570
HDDS-7941. Intermittent failure in testObjectStoreCreateWithO3fs.
Sep 8, 2023
0d08eef
HDDS-7941. Handled review comments.
Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,7 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
Table<String, OmKeyInfo> keyTable = metadataManager.getKeyTable(layout);
Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> cacheIterator =
keyTable.cacheIterator();
Set<String> deletedKeys = new HashSet<>();
while (cacheIterator.hasNext()) {
Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> cacheEntry =
cacheIterator.next();
Expand All @@ -1228,6 +1229,12 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
LOG.debug("Fake dir {} required for {}", targetKey, cacheKey);
return createDirectoryKey(cacheValue.getCacheValue(), dirKey);
}
// deletedKeys may contain deleted entry while iterating cache iterator
// To avoid race condition of flush of cache while iterating
// table iterator.
if (!exists) {
deletedKeys.add(cacheKey);
}
}
Copy link
Contributor

@smengcl smengcl Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This can be dumping a lot of keyTable cache entries into the temp set deletedKeys. And this method can be called frequently from getFileStatus / getOzoneFileStatus.

Is there a better way to fix this? Since the only caller is already holding a BUCKET_LOCK:

ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

Line 1127 in e4cbc20

metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,

It might be fine for now since keyTable cache should only hold a few entries that are not flushed yet.

Copy link
Contributor Author

@devmadhuu devmadhuu Sep 9, 2023

Choose a reason for hiding this comment

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

Thanks @smengcl for your review. Another way to fix is this #5093 PR. Here a table.isExist() API being used, but my only concern is that cache is always faster and isExist is being called in #5093 PR till we find the target key as well it is being called inside getNextKey() call. Pls have look #5093 PR

Copy link
Contributor Author

@devmadhuu devmadhuu Sep 12, 2023

Choose a reason for hiding this comment

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

@smengcl Kindly advise.

Copy link
Contributor

@smengcl smengcl Sep 22, 2023

Choose a reason for hiding this comment

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

Alright. Let's merge this PR now for a fix. We could improve this further in #5093 .


try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
Expand All @@ -1242,7 +1249,7 @@ private OmKeyInfo createFakeDirIfShould(String volume, String bucket,
// in different volume/bucket, such as "/vol1/bucket2/dir2/key2".
if (key.startsWith(targetKey)) {
if (!Objects.equals(key, targetKey)
&& !isKeyDeleted(key, keyTable)) {
&& !deletedKeys.contains(key)) {
LOG.debug("Fake dir {} required for {}", targetKey, key);
return createDirectoryKey(keyValue.getValue(), dirKey);
}
Expand Down
4 changes: 3 additions & 1 deletion ...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneListStatusHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,9 @@ public Collection<OzoneFileStatus> listStatusFSO(OmKeyArgs args,
HeapEntry entry = heapIterator.next();
OzoneFileStatus status = entry.getStatus(prefixKey,
scmBlockSize, volumeName, bucketName, replication);
map.putIfAbsent(entry.key, status);
if (!map.containsKey(entry.key)) {
map.put(entry.key, status);
}
}
}

Expand Down