Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1462,10 +1462,6 @@ public static boolean isKeyDeleted(String key, Table keyTable) {
&& omKeyInfoCacheValue.getCacheValue() == null;
}

public static boolean isKeyInCache(String key, Table keyTable) {
return keyTable.getCacheValue(new CacheKey(key)) != null;
}

/**
* Helper function for listStatus to find key in TableCache.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.NoSuchElementException;
import java.util.PriorityQueue;
import java.util.TreeMap;
import java.util.function.Predicate;

import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;

Expand Down Expand Up @@ -117,14 +118,17 @@ public static class DbTableIter<Value> implements
? extends Table.KeyValue<String, Value>> tableIterator;

private final Table<String, Value> table;
private HeapEntry currentKey;
private HeapEntry currentEntry;
private Predicate<String> doesKeyExistInCache;

DbTableIter(int entryIteratorId, Table<String, Value> table,
String prefixKey, String startKey) throws IOException {
String prefixKey, String startKey,
Predicate<String> doesKeyExistInCache) throws IOException {
this.entryIteratorId = entryIteratorId;
this.table = table;
this.tableIterator = table.iterator(prefixKey);
this.currentKey = null;
this.currentEntry = null;
this.doesKeyExistInCache = doesKeyExistInCache;

// only seek for the start key if the start key is lexicographically
// after the prefix key. For example
Expand All @@ -141,11 +145,11 @@ public static class DbTableIter<Value> implements
}

private void getNextKey() throws IOException {
while (tableIterator.hasNext() && currentKey == null) {
while (tableIterator.hasNext() && currentEntry == null) {
Table.KeyValue<String, Value> entry = tableIterator.next();
String entryKey = entry.getKey();
if (!KeyManagerImpl.isKeyInCache(entryKey, table)) {
currentKey = new HeapEntry(entryIteratorId,
if (!doesKeyExistInCache.test(entryKey)) {
currentEntry = new HeapEntry(entryIteratorId,
table.getName(), entryKey, entry.getValue());
}
}
Expand All @@ -157,13 +161,13 @@ public boolean hasNext() {
} catch (IOException t) {
throw new UncheckedIOException(t);
}
return currentKey != null;
return currentEntry != null;
}

public HeapEntry next() {
if (hasNext()) {
HeapEntry ret = currentKey;
currentKey = null;
HeapEntry ret = currentEntry;
currentEntry = null;
return ret;
}
throw new NoSuchElementException();
Expand All @@ -186,15 +190,15 @@ public static class CacheIter<Value>
private final String prefixKey;
private final String startKey;
private final String tableName;

private HeapEntry currentEntry;
private final int entryIteratorId;

CacheIter(int entryIteratorId, String tableName,
Iterator<Map.Entry<CacheKey<String>,
CacheValue<Value>>> cacheIter, String startKey,
String prefixKey) {
this.cacheKeyMap = new TreeMap<>();

this.currentEntry = null;
this.startKey = startKey;
this.prefixKey = prefixKey;
this.tableName = tableName;
Expand Down Expand Up @@ -236,14 +240,37 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
}
}

public boolean doesKeyExistInCache(String key) {
return cacheKeyMap.containsKey(key);
}

private void getNextKey() throws IOException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: instead of creating a new function, we can just remove null values when initializing the cacheCreatedKeyIterator.
cacheCreatedKeyIter = cacheKeyMap.entrySet().stream().filter(e -> e.getValue() != null).iterator();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can not remove null value in cache iterator initialization as this is requried while checking db entry with cache if element is deleted or not (as deletion is marked with null value)
Optimization is done not to return null value while checking hasNext() or next(), so caller need not do check with null value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cacheCreatedKeyIter = cacheKeyMap.entrySet().stream().filter(e -> e.getValue() != null).iterator();
This should not remove any element from the map.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think @swamirishi is right. doesKeyExistInCache checks if key exist in cacheKeyMap while cacheCreatedKeyIter can have filtered result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, this filtering can be done while initializing iterator.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @hemantk-12 @swamirishi @sumitagrawl for review. This has been handled as per suggestion. Kindly re-review. Once changes finalized. I'll re-run the repeated CI run.

while (cacheCreatedKeyIter.hasNext() && currentEntry == null) {
Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
if (null == entry.getValue()) {

@swamirishi swamirishi Jan 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sumitagrawl @devmadhuu I am surely missing something here. Correct me if I am wrong, we are doing a continue here as per this statement which will skip the null value here. So all I am asking is when we are initializing cacheCreatedKeyIter can we skip null values? The key would be still there in the cacheKeyMap. It will make this code much simpler.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sumitagrawl @devmadhuu I am surely missing something here. Correct me if I am wrong, we are doing a continue here as per this statement which will skip the null value here. So all I am asking is when we are initializing cacheCreatedKeyIter can we skip null values? The key would be still there in the cacheKeyMap. It will make this code much simpler.

cacheKeyMap: This is used for 2 porpose,

  1. check if latest update available in cache compared to DB,
  • Value if not null: latest value from cache
  • Value if null: represent data is deleted and dbIterator check for this also

Now if we remove null value while initialize, then how DBIterator knows if keys are deleted? So this is required if key is deleted to skip from db iterator and hence we need this information.

  1. retrieve data in getNextKey() to caller.
    Here, null value is not required to be returned while iterator, so we can skip as above.

@swamirishi I think this will help to understand why we can not remove null value during initialization. else if any key deleted, this can not be know via dbIterator (as may not yet flushed to db with delete) and may give deleted key details to user.

continue;
}
currentEntry = new HeapEntry(this.entryIteratorId, this.tableName,
entry.getKey(), entry.getValue());
}
}

public boolean hasNext() {
return cacheCreatedKeyIter.hasNext();
try {
getNextKey();
} catch (IOException t) {
throw new UncheckedIOException(t);
}
return currentEntry != null;
}

public HeapEntry next() {
Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
return new HeapEntry(this.entryIteratorId, this.tableName,
entry.getKey(), entry.getValue());
if (hasNext()) {
HeapEntry ret = currentEntry;
currentEntry = null;
return ret;
}
throw new NoSuchElementException();
}

public void close() {
Expand Down Expand Up @@ -292,11 +319,13 @@ public static class MinHeapIterator implements ClosableIterator {
try {
int iteratorId = 0;
for (Table table : tables) {
iterators.add(new CacheIter<>(iteratorId, table.getName(),
table.cacheIterator(), startKey, prefixKey));
CacheIter cacheIter = new CacheIter<>(iteratorId, table.getName(),
table.cacheIterator(), startKey, prefixKey);
Predicate<String> doesKeyExistInCache = cacheIter::doesKeyExistInCache;
iterators.add(cacheIter);
iteratorId++;
iterators.add(new DbTableIter<>(iteratorId, table, prefixKey,
startKey));
startKey, doesKeyExistInCache));
iteratorId++;
}
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1372,8 +1372,8 @@ public List<SnapshotInfo> listSnapshot(
bucketName, snapshotInfoTable)) {
try {
while (snapshotIterator.hasNext() && maxListResult > 0) {
SnapshotInfo snapshotInfo = (SnapshotInfo) snapshotIterator.next()
.getValue();
SnapshotInfo snapshotInfo =
(SnapshotInfo) snapshotIterator.next().getValue();
if (!snapshotInfo.getName().equals(prevSnapshot)) {
snapshotInfos.add(snapshotInfo);
maxListResult--;
Expand Down