HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. #5970

devmadhuu · 2024-01-10T07:08:28Z

What changes were proposed in this pull request?

This PR fixes the Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot.

TestOzoneRpcClientAbstract.testListSnapshot creates 20 snapshots using createSnapshot API and asserts the count of same using listSnapshot API. This assertion of count fails intermittently.

listSnapshot API uses the org.apache.hadoop.ozone.om.ListIterator.MinHeapIterator which internally uses both CacheIterator and DBIterator and DBIterator had the logic of checking if rocks DB key is present in cache in org.apache.hadoop.ozone.om.ListIterator.DbTableIter#getNextKey, this checks the cache from table cache which may be intermittently flushed which makes the addition of duplicate entry in org.apache.hadoop.ozone.om.ListIterator.MinHeapIterator. So to fix this, we should use the pre-loaded keys in org.apache.hadoop.ozone.om.ListIterator.CacheIter#cacheKeyMap in org.apache.hadoop.ozone.om.ListIterator.CacheIter.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch is tested by repeated CI runs around 500 iterations and ZERO failure reported for this test case. Here is the green CI link.

…tSnapshot.

devmadhuu · 2024-01-10T07:08:44Z

@sumitagrawl @adoroszlai Pls review.

hemantk-12

Thanks @devmadhuu for the patch and finding out the root cause.

Overall looks good to me, left some inline comments.

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

sumitagrawl

@devmadhuu thanks for working over this. Have few comments.
Additionally, can avoid returning null value in next() with some optimization.

swamirishi

Thanks for the patch @devmadhuu Overall the changes look good to me I had some nitpicky comment in the one place, check and see if it makes sense.

swamirishi · 2024-01-11T20:50:08Z

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

+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {


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();

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.

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

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

yes, this filtering can be done while initializing iterator.

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.

hemantk-12

Overall looks good to me.

hemantk-12 · 2024-01-11T21:01:58Z

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

    private final String startKey;
    private final String tableName;
-
+    private HeapEntry currentKey;


nit: currentEntry and getCurrentEntry()

hemantk-12 · 2024-01-11T21:03:35Z

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

          CacheIter cacheIter = new CacheIter<>(iteratorId, table.getName(),
              table.cacheIterator(), startKey, prefixKey);
-          Predicate<String> isKeyExistInCache = cacheIter::isKeyExistInCache;
+          Predicate<String> doesKeyExistInCache =


nit: JFYI, now char limit is 120. It can be fit in one line. Configure/update your IDE with latest ozone-style.

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

sumitagrawl

@devmadhuu Overall looks good, have one minor comment

sumitagrawl

@devmadhuu LGTM +1

devmadhuu · 2024-01-16T05:55:52Z

@swamirishi @hemantk-12 Kindly re-review.

swamirishi · 2024-01-16T16:11:35Z

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

+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {


@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.

@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,

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.

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.

hemantk-12

LGTM+1.

swamirishi

I have a minor nitpicky comment on the iterator initialization. You can make the changes if you feel it is good.

swamirishi · 2024-01-16T16:12:57Z

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

+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {


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

sumitagrawl · 2024-01-17T06:35:58Z

I have a minor nitpicky comment on the iterator initialization. You can make the changes if you feel it is good.

I think above explaination gives why null value key is requried and help to avoid returning key which is deleted in cache.

devmadhuu · 2024-01-17T10:01:19Z

@sumitagrawl @swamirishi Pls re-review.

devmadhuu · 2024-01-17T10:02:20Z

I have a minor nitpicky comment on the iterator initialization. You can make the changes if you feel it is good.

Thanks @swamirishi for your review. Changes done as suggested. Kindly re-review.

sumitagrawl

LGTM

hemantk-12

Thanks @devmadhuu for the patch and incorporating the review comments.

LGTM.

swamirishi

@devmadhuu Thanks for the changes LGTM

adoroszlai · 2024-01-17T21:47:36Z

Thanks @devmadhuu for the patch, @hemantk-12, @sumitagrawl, @swamirishi for the review.

…tSnapshot. (apache#5970) (cherry picked from commit fa98426) (cherry picked from commit 42e8540f3f3fcfadfbc4e66579fa2465a2eb3725)

…ract.testListSnapshot. (apache#5970) (cherry picked from commit fa98426) Change-Id: Id59434644bfb9bbbeb2515dd4ecdd1bdad90fef7

HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testLis…

2cb8bac

…tSnapshot.

adoroszlai added the test label Jan 10, 2024

adoroszlai requested review from hemantk-12, sumitagrawl and swamirishi January 10, 2024 07:42

adoroszlai added snapshot https://issues.apache.org/jira/browse/HDDS-6517 and removed test labels Jan 10, 2024

hemantk-12 reviewed Jan 10, 2024

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java Outdated Show resolved Hide resolved

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java Outdated Show resolved Hide resolved

sumitagrawl reviewed Jan 11, 2024

View reviewed changes

HDDS-9967. Fixed Review Comments.

a226092

swamirishi requested changes Jan 11, 2024

View reviewed changes

hemantk-12 reviewed Jan 11, 2024

View reviewed changes

sumitagrawl reviewed Jan 12, 2024

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java Outdated Show resolved Hide resolved

sumitagrawl reviewed Jan 12, 2024

View reviewed changes

HDDS-9967. Fixed Review Comments.

5202873

sumitagrawl approved these changes Jan 12, 2024

View reviewed changes

swamirishi reviewed Jan 16, 2024

View reviewed changes

hemantk-12 reviewed Jan 16, 2024

View reviewed changes

swamirishi reviewed Jan 16, 2024

View reviewed changes

HDDS-9967. Fixed Review Comments.

6709235

HDDS-9967. Fixed Review Comments.

dee1298

sumitagrawl approved these changes Jan 17, 2024

View reviewed changes

hemantk-12 approved these changes Jan 17, 2024

View reviewed changes

adoroszlai added test and removed test labels Jan 17, 2024

adoroszlai requested a review from swamirishi January 17, 2024 19:47

swamirishi approved these changes Jan 17, 2024

View reviewed changes

adoroszlai merged commit fa98426 into apache:master Jan 17, 2024

HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. #5970

HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. #5970

Uh oh!

Conversation

devmadhuu commented Jan 10, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

devmadhuu commented Jan 10, 2024

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Jan 16, 2024

Uh oh!

swamirishi Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitagrawl commented Jan 17, 2024

Uh oh!

devmadhuu commented Jan 17, 2024

Uh oh!

devmadhuu commented Jan 17, 2024

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi Jan 16, 2024 •

edited

Loading