[GitHub] [hadoop-ozone] sodonnel commented on pull request #1111: HDDS-3854. Fix error return value of KeyValueBlockIterator#hasNext

[GitBox <gi...@apache.org>]

sodonnel commented on pull request #1111:
URL: https://github.com/apache/hadoop-ozone/pull/1111#issuecomment-648797267


   Good catch @runzhiwang 
   
   It seems to me that the iterator fails to work correctly if there is a filter which filters out any of the blocks. The first time a block is filtered out, even though there are recursive calls, it will still return false at the top level and the iterator will stop, even if there are more blocks available.
   
   I was surprised a unit test did not catch this, so I checked TestKeyValueBlockIterator#testKeyValueBlockIteratorWithFilter and it fails to catch this:
   
   ```
     @Test
     public void testKeyValueBlockIteratorWithFilter() throws Exception {
       long containerId = 103L;
       int deletedBlocks = 5;
       int normalBlocks = 5;
       createContainerWithBlocks(containerId, normalBlocks, deletedBlocks);
       String containerPath = new File(containerData.getMetadataPath())
           .getParent();
       try(KeyValueBlockIterator keyValueBlockIterator = new KeyValueBlockIterator(
           containerId, new File(containerPath), MetadataKeyFilters
           .getDeletingKeyFilter())) {
   
         int counter = 5;
         while (keyValueBlockIterator.hasNext()) {
           BlockData blockData = keyValueBlockIterator.nextBlock();
           assertEquals(blockData.getLocalID(), counter++);
         }
       }
     }
   ```
   The reason it fails to catch it is because it does not check `count` is incremented from 5 to 10, as it expects to get the 5 deleting blocks. If we add an assertEquals(10, count) at the end of the test, it would catch this issue. However there is another problem in the test, as the createContainerWithBlocks method never created the "deleting" blocks.
   
   With the following change to the test I can reproduce the issue and your change fixes it:
   
   ```
   diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java
   index ccd3227fa..1f1423264 100644
   --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java
   +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueBlockIterator.java
   @@ -223,6 +223,7 @@ public void testKeyValueBlockIteratorWithFilter() throws Exception {
            BlockData blockData = keyValueBlockIterator.nextBlock();
            assertEquals(blockData.getLocalID(), counter++);
          }
   +      assertEquals(10, counter);
        }
      }
    
   @@ -276,8 +277,8 @@ private void createContainerWithBlocks(long containerId, int
                .getProtoBufMessage().toByteArray());
          }
    
   -      for (int i = normalBlocks; i < deletedBlocks; i++) {
   -        BlockID blockID = new BlockID(containerId, i);
   +      for (int i = 0; i < deletedBlocks; i++) {
   +        BlockID blockID = new BlockID(containerId, i + normalBlocks);
            BlockData blockData = new BlockData(blockID);
            blockData.setChunks(chunkList);
            metadataStore.getStore().put(StringUtils.string2Bytes(OzoneConsts
   ```
   
   Could you make that change to the tests and then we are good to commit this change. Thanks.
   
   I was also wondering - how did you discover this? Did you discover a problem at runtime on your cluster which caused you to investigate this?


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

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



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