You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "devmadhuu (via GitHub)" <gi...@apache.org> on 2024/01/10 07:08:30 UTC

[PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

devmadhuu opened a new pull request, #5970:
URL: https://github.com/apache/ozone/pull/5970

   ## 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](https://github.com/devmadhuu/ozone/actions/runs/7470560512/job/20330782349).
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1455179211


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

Review Comment:
   yes, this filtering can be done while initializing iterator.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1454045492


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

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



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1453652570


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {

Review Comment:
   @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 we can skip null values. The key would be still there in the cacheKeyMap. It will make this code much simpler.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1884301257

   @sumitagrawl @adoroszlai Pls review.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449780382


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

Review Comment:
   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.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1895473664

   @sumitagrawl @swamirishi Pls re-review.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449366921


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +332,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because

Review Comment:
   Yeah we need the null values



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449389595


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -191,15 +191,15 @@ public static class CacheIter<Value>
     private final String prefixKey;
     private final String startKey;
     private final String tableName;
-
+    private HeapEntry currentKey;

Review Comment:
   nit: `currentEntry` and `getCurrentEntry()`



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -303,11 +323,12 @@ public static class MinHeapIterator implements ClosableIterator {
         for (Table table : tables) {
           CacheIter cacheIter = new CacheIter<>(iteratorId, table.getName(),
               table.cacheIterator(), startKey, prefixKey);
-          Predicate<String> isKeyExistInCache = cacheIter::isKeyExistInCache;
+          Predicate<String> doesKeyExistInCache =

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



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449781554


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +353,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because
+    // value of key would be returned as null from cache iterator.
+    public @Nullable HeapEntry next() {

Review Comment:
   we can remove @Nullable here from next



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449841871


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -191,15 +191,15 @@ public static class CacheIter<Value>
     private final String prefixKey;
     private final String startKey;
     private final String tableName;
-
+    private HeapEntry currentKey;

Review Comment:
   Done.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1454711047


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {

Review Comment:
   > @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.
   
   2. 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.
   
   
   



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1453652570


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {

Review Comment:
   @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.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1893114155

   @swamirishi @hemantk-12 Kindly re-review.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1895475396

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


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1453652570


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {

Review Comment:
   @sumitagrawl 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 we can skip null values. The key would be still there in the cacheKeyMap. It will make this code much simpler.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {
+      while (cacheCreatedKeyIter.hasNext() && currentKey == null) {
+        Map.Entry<String, Value> entry = cacheCreatedKeyIter.next();
+        if (null == entry.getValue()) {

Review Comment:
   @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 we can skip null values. The key would be still there in the cacheKeyMap. It will make this code much simpler.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449379760


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

Review Comment:
   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();`



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1448449510


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +332,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because

Review Comment:
   This information is required while checking db entry, as key is deleted in cache but not yet flushed to db, so db can have these entry. To avoid returning db entry, can not filter out.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1448464239


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -144,7 +149,7 @@ private void getNextKey() throws IOException {
       while (tableIterator.hasNext() && currentKey == null) {
         Table.KeyValue<String, Value> entry = tableIterator.next();
         String entryKey = entry.getKey();
-        if (!KeyManagerImpl.isKeyInCache(entryKey, table)) {
+        if (!isKeyExistsInCache.test(entryKey)) {

Review Comment:
   seems some problem in implementation of DBTableIterator,
   - hasNext() moves to next key by calling getNextKey()
   - next() also call hasNext() implicitly calling getNextKey() and moves to next key
   So while iterator, element can move twice skipping entries
   



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1447942961


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +332,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because

Review Comment:
   I didn't quite get this comment. Does it mean for deleted entries cache will have null value? If that's the case, I think better approach would be to filter out entries when populating CacheIter in `populateCacheMap`.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449814105


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +353,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because
+    // value of key would be returned as null from cache iterator.
+    public @Nullable HeapEntry next() {

Review Comment:
   ok.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1447942961


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +332,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because

Review Comment:
   I didn't quite get this comment. Does it mean for deleted entries cache will have null value? If that's the case, I think better approach would be to filter out entries when populating in `CacheIter#populateCacheMap`.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1453654292


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

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



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1895070533

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


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #5970:
URL: https://github.com/apache/ozone/pull/5970#issuecomment-1896932339

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


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1449831655


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -303,11 +323,12 @@ public static class MinHeapIterator implements ClosableIterator {
         for (Table table : tables) {
           CacheIter cacheIter = new CacheIter<>(iteratorId, table.getName(),
               table.cacheIterator(), startKey, prefixKey);
-          Predicate<String> isKeyExistInCache = cacheIter::isKeyExistInCache;
+          Predicate<String> doesKeyExistInCache =

Review Comment:
   ok done.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1448464239


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -144,7 +149,7 @@ private void getNextKey() throws IOException {
       while (tableIterator.hasNext() && currentKey == null) {
         Table.KeyValue<String, Value> entry = tableIterator.next();
         String entryKey = entry.getKey();
-        if (!KeyManagerImpl.isKeyInCache(entryKey, table)) {
+        if (!isKeyExistsInCache.test(entryKey)) {

Review Comment:
   seems some problem in implementation of DBTableIterator,
   - hasNext() moves to next key by calling getNextKey()
   - next() also call hasNext() implicitly calling getNextKey() and moves to next key
   So while iterator, element can move twice skipping entries
   



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1447937609


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -118,13 +120,16 @@ public static class DbTableIter<Value> implements
 
     private final Table<String, Value> table;
     private HeapEntry currentKey;
+    private Predicate<String> isKeyExistsInCache;

Review Comment:
   ```suggestion
       private Predicate<String> isKeyExistInCache;
   ```
   
   nit: may be name is `doesKeyExistInCache`.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -321,7 +332,9 @@ public boolean hasNext() {
       return !minHeap.isEmpty();
     }
 
-    public HeapEntry next() {
+    // HeapEntry can be null when a key is marked as deleted because

Review Comment:
   I didn't quite get this comment. Does it mean for deleted entries cache will have null value? If that's the case, I think better approach would be to filter it out when populating CacheIter in `populateCacheMap`.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #5970:
URL: https://github.com/apache/ozone/pull/5970


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-9967. Intermittent failure in TestOzoneRpcClientAbstract.testListSnapshot. [ozone]

Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5970:
URL: https://github.com/apache/ozone/pull/5970#discussion_r1455207562


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java:
##########
@@ -236,14 +241,38 @@ private void populateCacheMap(Iterator<Map.Entry<CacheKey<String>,
       }
     }
 
+    public boolean doesKeyExistInCache(String key) {
+      return cacheKeyMap.containsKey(key);
+    }
+
+    private void getNextKey() throws IOException {

Review Comment:
   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.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org