You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/06/16 03:01:06 UTC

[GitHub] [hadoop-ozone] ChenSammi opened a new pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

ChenSammi opened a new pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079


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


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

[GitHub] [hadoop-ozone] ChenSammi commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r441387604



##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestKeyInputStream.java
##########
@@ -331,4 +351,45 @@ public void testCopyLarge() throws Exception {
       }
     }
   }
+
+  @Test
+  public void testReadChunk() throws Exception {
+    String keyName = getKeyName();
+    OzoneOutputStream key = TestHelper.createKey(keyName,
+        ReplicationType.RATIS, 0, objectStore, volumeName, bucketName);
+
+    // write data spanning multiple chunks
+    int dataLength = (2 * chunkSize) + (chunkSize / 2);
+    byte[] originData = new byte[dataLength];
+    Random r = new Random();
+    r.nextBytes(originData);
+    key.write(originData);
+    key.close();
+
+    // read chunk data
+    KeyInputStream keyInputStream = (KeyInputStream) objectStore
+        .getVolume(volumeName).getBucket(bucketName).readKey(keyName)
+        .getInputStream();
+
+    int[] bufferSizeList = {chunkSize / 4, chunkSize / 2, chunkSize - 1,
+        chunkSize, chunkSize + 1, blockSize - 1, blockSize, blockSize + 1,
+        blockSize * 2};
+    for (int bufferSize : bufferSizeList) {
+      byte[] data = new byte[bufferSize];
+      int totalRead = 0;
+      while (totalRead < dataLength) {
+        int numBytesRead = keyInputStream.read(data);
+        if (numBytesRead == -1 || numBytesRead == 0) {
+          break;
+        }
+        byte[] tmp1 =
+            Arrays.copyOfRange(originData, totalRead, totalRead + numBytesRead);
+        byte[] tmp2 =
+            Arrays.copyOfRange(data, 0, numBytesRead);
+        Arrays.equals(tmp1, tmp2);

Review comment:
       Thanks.




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

[GitHub] [hadoop-ozone] ChenSammi commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-646401732


   Thanks @adoroszlai  and @bshashikant 


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

[GitHub] [hadoop-ozone] ChenSammi commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r441326124



##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestKeyInputStream.java
##########
@@ -64,25 +71,38 @@
   private static String volumeName;
   private static String bucketName;
   private static String keyString;
+  private static ChunkLayoutTestInfo chunkLayout;
+
+  @Parameterized.Parameters
+  public static Collection<Object[]> layouts() {
+    return Arrays.asList(new Object[][] {
+        {ChunkLayoutTestInfo.FILE_PER_CHUNK},
+        {ChunkLayoutTestInfo.FILE_PER_BLOCK}
+    });
+  }
 
+  public TestKeyInputStream(ChunkLayoutTestInfo layout) {
+    this.chunkLayout = layout;
+  }
   /**
    * Create a MiniDFSCluster for testing.
    * <p>
    * Ozone is made active by setting OZONE_ENABLED = true
    *
    * @throws IOException
    */
-  @BeforeClass
-  public static void init() throws Exception {
-    chunkSize = 100;
+  @Before
+  public void init() throws Exception {
+    chunkSize = 1024 * 1024 * 4;

Review comment:
       I will try to reduce the chunkSize. One limitation here is chunkSize cann't be less than the mininum bytes-per-checksume value which is 256KB, otherwise the TestKeyInputStream will always succeed event without this patch. 




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

[GitHub] [hadoop-ozone] ChenSammi commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r441262440



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +244,36 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    int index = extractChunkIndex(info.getChunkName());
+    if (index == -1) {
+      throw new StorageContainerException(
+          "Chunk file name can't be parsed " + possibleFiles.toString(),
+          UNABLE_TO_FIND_CHUNK);
+    }
+    // Chunk index start from 1
+    Preconditions.checkState(index > 0);
+
+    long chunkFileOffset;
+    try {
+      BlockData blockData = blockManager.getBlock(kvContainer, blockID);
+      List<ContainerProtos.ChunkInfo> chunks = blockData.getChunks();
+      Preconditions.checkState(index <= chunks.size());
+      chunkFileOffset = chunks.get(index - 1).getOffset();

Review comment:
       A good point. I think the performance overhead can be ignored. 




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

[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-646029953


   > @adoroszlai thanks for the review. Is it a +1?
   
   Yes.
   
   > Should I wait for @bshashikant 's review and feedback before commit?
   
   Let's wait a bit more.


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

[GitHub] [hadoop-ozone] bshashikant commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r442257754



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +222,40 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    long chunkFileOffset = 0;
+    if (info.getOffset() != 0) {
+      try {
+        BlockData blockData = blockManager.getBlock(kvContainer, blockID);
+        List<ContainerProtos.ChunkInfo> chunks = blockData.getChunks();
+        String chunkName = info.getChunkName();
+        boolean found = false;
+        for (ContainerProtos.ChunkInfo chunk : chunks) {
+          if (chunk.getChunkName().equals(chunkName)) {
+            chunkFileOffset = chunk.getOffset();
+            found = true;
+            break;
+          }
+        }
+        if (!found) {
+          throw new StorageContainerException(
+              "Cannot find chunk " + chunkName + " in block " +
+                  blockID.toString(), UNABLE_TO_FIND_CHUNK);
+        }
+      } catch (IOException e) {
+        throw new StorageContainerException(
+            "Cannot find block " + blockID.toString() + " for chunk " +
+                info.getChunkName(), UNABLE_TO_FIND_CHUNK);
+      }
+    }
+
     for (File file : possibleFiles) {
       try {
-        // use offset only if file written by old datanode
-        long offset;
-        if (file.exists() && file.length() == info.getOffset() + len) {
-          offset = info.getOffset();
-        } else {
-          offset = 0;
+        if (file.exists()) {
+          long offset = info.getOffset() - chunkFileOffset;
+          Preconditions.checkState(offset >= 0);

Review comment:
       I think, the precondition here won't hold good if we are using older client (prior FILE_PER_BLOCK strategy where offset for each chunk would be 0). @ChenSammi do you have a requirement for 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

[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r440669964



##########
File path: hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/AbstractTestChunkManager.java
##########
@@ -62,14 +63,17 @@
   private ChunkInfo chunkInfo;
   private ByteBuffer data;
   private byte[] header;
+  private BlockManager blockManager;
+  private final String chunkFileNamePattern = "%d_data_%d";

Review comment:
       Nit: I think it can be a `public static` constant.  It's not instance-specific, and it also makes the accessor method unnecessary.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +244,36 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    int index = extractChunkIndex(info.getChunkName());
+    if (index == -1) {
+      throw new StorageContainerException(
+          "Chunk file name can't be parsed " + possibleFiles.toString(),
+          UNABLE_TO_FIND_CHUNK);
+    }
+    // Chunk index start from 1
+    Preconditions.checkState(index > 0);
+
+    long chunkFileOffset;
+    try {
+      BlockData blockData = blockManager.getBlock(kvContainer, blockID);
+      List<ContainerProtos.ChunkInfo> chunks = blockData.getChunks();
+      Preconditions.checkState(index <= chunks.size());
+      chunkFileOffset = chunks.get(index - 1).getOffset();

Review comment:
       Instead of parsing the chunk name for an index, we could find the chunk with the exact same name.  I think it's less fragile, although might be slower if the block has too many chunks.  What's your opinion?
   
   https://github.com/adoroszlai/hadoop-ozone/commit/398ea1d5991c44baa59c120dea8fa3c80727e9e2 (note: CI is still in progress, may need tweak based on the result)

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -34,11 +34,14 @@
 import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
 import org.apache.hadoop.ozone.container.common.volume.VolumeIOStats;
 import org.apache.hadoop.ozone.container.keyvalue.helpers.ChunkUtils;
+import org.apache.hadoop.ozone.container.keyvalue.interfaces.BlockManager;
 import org.apache.hadoop.ozone.container.keyvalue.interfaces.ChunkManager;
 import org.apache.hadoop.ozone.container.common.interfaces.Container;
 
 import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.Result.IO_EXCEPTION;
 import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.Result.UNABLE_TO_FIND_CHUNK;
+
+import org.eclipse.jetty.util.StringUtil;

Review comment:
       Nit: Can we please avoid this dependency?  I think it's better to use `StringUtils` from Apache Commons Lang, or even simply check explicitly both for null and empty string.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +244,36 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    int index = extractChunkIndex(info.getChunkName());
+    if (index == -1) {
+      throw new StorageContainerException(
+          "Chunk file name can't be parsed " + possibleFiles.toString(),
+          UNABLE_TO_FIND_CHUNK);
+    }
+    // Chunk index start from 1
+    Preconditions.checkState(index > 0);
+
+    long chunkFileOffset;
+    try {
+      BlockData blockData = blockManager.getBlock(kvContainer, blockID);

Review comment:
       I think we can skip trying to determine chunk file offset if `info.getOffset() == 0`.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +244,36 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    int index = extractChunkIndex(info.getChunkName());
+    if (index == -1) {
+      throw new StorageContainerException(
+          "Chunk file name can't be parsed " + possibleFiles.toString(),
+          UNABLE_TO_FIND_CHUNK);
+    }
+    // Chunk index start from 1
+    Preconditions.checkState(index > 0);
+
+    long chunkFileOffset;
+    try {
+      BlockData blockData = blockManager.getBlock(kvContainer, blockID);
+      List<ContainerProtos.ChunkInfo> chunks = blockData.getChunks();
+      Preconditions.checkState(index <= chunks.size());
+      chunkFileOffset = chunks.get(index - 1).getOffset();
+    } catch (IOException e) {
+      throw new StorageContainerException(
+          "Cannot find block " + blockID.toString() + " for chunk " +
+              info.getChunkName(), UNABLE_TO_FIND_CHUNK);
+    }
+
     for (File file : possibleFiles) {
       try {
-        // use offset only if file written by old datanode
-        long offset;
-        if (file.exists() && file.length() == info.getOffset() + len) {
-          offset = info.getOffset();
-        } else {
-          offset = 0;
+        if (file.exists()) {
+          long offset = info.getOffset() - chunkFileOffset;
+          Preconditions.checkState(offset < file.length());
+          Preconditions.checkState((offset + len) <= file.length());

Review comment:
       Please store `file.length()` in local variable to avoid duplicate call.
   
   Also, should we consider moving these checks to `ChunkUtils.readData`?

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestKeyInputStream.java
##########
@@ -331,4 +351,45 @@ public void testCopyLarge() throws Exception {
       }
     }
   }
+
+  @Test
+  public void testReadChunk() throws Exception {
+    String keyName = getKeyName();
+    OzoneOutputStream key = TestHelper.createKey(keyName,
+        ReplicationType.RATIS, 0, objectStore, volumeName, bucketName);
+
+    // write data spanning multiple chunks
+    int dataLength = (2 * chunkSize) + (chunkSize / 2);
+    byte[] originData = new byte[dataLength];
+    Random r = new Random();
+    r.nextBytes(originData);
+    key.write(originData);
+    key.close();
+
+    // read chunk data
+    KeyInputStream keyInputStream = (KeyInputStream) objectStore
+        .getVolume(volumeName).getBucket(bucketName).readKey(keyName)
+        .getInputStream();
+
+    int[] bufferSizeList = {chunkSize / 4, chunkSize / 2, chunkSize - 1,
+        chunkSize, chunkSize + 1, blockSize - 1, blockSize, blockSize + 1,
+        blockSize * 2};
+    for (int bufferSize : bufferSizeList) {
+      byte[] data = new byte[bufferSize];
+      int totalRead = 0;
+      while (totalRead < dataLength) {
+        int numBytesRead = keyInputStream.read(data);
+        if (numBytesRead == -1 || numBytesRead == 0) {
+          break;
+        }
+        byte[] tmp1 =
+            Arrays.copyOfRange(originData, totalRead, totalRead + numBytesRead);
+        byte[] tmp2 =
+            Arrays.copyOfRange(data, 0, numBytesRead);
+        Arrays.equals(tmp1, tmp2);

Review comment:
       ```suggestion
           Assert.assertArrayEquals(tmp1, tmp2);
           totalRead += numBytesRead;
   ```

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestKeyInputStream.java
##########
@@ -64,25 +71,38 @@
   private static String volumeName;
   private static String bucketName;
   private static String keyString;
+  private static ChunkLayoutTestInfo chunkLayout;
+
+  @Parameterized.Parameters
+  public static Collection<Object[]> layouts() {
+    return Arrays.asList(new Object[][] {
+        {ChunkLayoutTestInfo.FILE_PER_CHUNK},
+        {ChunkLayoutTestInfo.FILE_PER_BLOCK}
+    });
+  }
 
+  public TestKeyInputStream(ChunkLayoutTestInfo layout) {
+    this.chunkLayout = layout;
+  }
   /**
    * Create a MiniDFSCluster for testing.
    * <p>
    * Ozone is made active by setting OZONE_ENABLED = true
    *
    * @throws IOException
    */
-  @BeforeClass
-  public static void init() throws Exception {
-    chunkSize = 100;
+  @Before
+  public void init() throws Exception {
+    chunkSize = 1024 * 1024 * 4;

Review comment:
       It seems the combination of increasing chunk size, creating new cluster for each test method, and running for both chunk layouts makes this integration test flaky: https://github.com/apache/hadoop-ozone/pull/1079/checks?check_run_id=775565603
   
   Can we use KB instead of MB for each size setting?
   
   https://github.com/adoroszlai/hadoop-ozone/commit/0cc9272c5f6f3666fe354aaee89010a9c22c8ca4




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

[GitHub] [hadoop-ozone] bshashikant commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r442257754



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerChunkStrategy.java
##########
@@ -218,17 +222,40 @@ public ChunkBuffer readChunk(Container container, BlockID blockID,
     long len = info.getLen();
     ByteBuffer data = ByteBuffer.allocate((int) len);
 
+    long chunkFileOffset = 0;
+    if (info.getOffset() != 0) {
+      try {
+        BlockData blockData = blockManager.getBlock(kvContainer, blockID);
+        List<ContainerProtos.ChunkInfo> chunks = blockData.getChunks();
+        String chunkName = info.getChunkName();
+        boolean found = false;
+        for (ContainerProtos.ChunkInfo chunk : chunks) {
+          if (chunk.getChunkName().equals(chunkName)) {
+            chunkFileOffset = chunk.getOffset();
+            found = true;
+            break;
+          }
+        }
+        if (!found) {
+          throw new StorageContainerException(
+              "Cannot find chunk " + chunkName + " in block " +
+                  blockID.toString(), UNABLE_TO_FIND_CHUNK);
+        }
+      } catch (IOException e) {
+        throw new StorageContainerException(
+            "Cannot find block " + blockID.toString() + " for chunk " +
+                info.getChunkName(), UNABLE_TO_FIND_CHUNK);
+      }
+    }
+
     for (File file : possibleFiles) {
       try {
-        // use offset only if file written by old datanode
-        long offset;
-        if (file.exists() && file.length() == info.getOffset() + len) {
-          offset = info.getOffset();
-        } else {
-          offset = 0;
+        if (file.exists()) {
+          long offset = info.getOffset() - chunkFileOffset;
+          Preconditions.checkState(offset >= 0);

Review comment:
       I think, the precondition here won't hold good if we are using older client (prior FILE_PER_BLOCK strategy where offset for each chunk would be 0). @ChenSammi do you have a requirement for 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

[GitHub] [hadoop-ozone] bshashikant merged pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
bshashikant merged pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079


   


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

[GitHub] [hadoop-ozone] codecov-commenter commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-644519933


   # [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=h1) Report
   > Merging [#1079](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=desc) into [master](https://codecov.io/gh/apache/hadoop-ozone/commit/2505b5c4d735a18c1e286c5d7281faf1f288c7bf&el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `58.97%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/graphs/tree.svg?width=650&height=150&src=pr&token=5YeeptJMby)](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1079      +/-   ##
   ============================================
   + Coverage     70.41%   70.44%   +0.03%     
   - Complexity     9241     9245       +4     
   ============================================
     Files           961      961              
     Lines         48132    48162      +30     
     Branches       4676     4683       +7     
   ============================================
   + Hits          33894    33930      +36     
   + Misses        11983    11976       -7     
   - Partials       2255     2256       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [.../container/keyvalue/impl/FilePerChunkStrategy.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvaW1wbC9GaWxlUGVyQ2h1bmtTdHJhdGVneS5qYXZh) | `60.74% <54.28%> (-8.79%)` | `16.00 <2.00> (-1.00)` | |
   | [...doop/ozone/container/keyvalue/KeyValueHandler.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvS2V5VmFsdWVIYW5kbGVyLmphdmE=) | `61.55% <100.00%> (-0.45%)` | `63.00 <0.00> (-1.00)` | |
   | [...ontainer/keyvalue/impl/ChunkManagerDispatcher.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvaW1wbC9DaHVua01hbmFnZXJEaXNwYXRjaGVyLmphdmE=) | `80.00% <100.00%> (ø)` | `9.00 <0.00> (ø)` | |
   | [...e/container/keyvalue/impl/ChunkManagerFactory.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29udGFpbmVyLXNlcnZpY2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2hhZG9vcC9vem9uZS9jb250YWluZXIva2V5dmFsdWUvaW1wbC9DaHVua01hbmFnZXJGYWN0b3J5LmphdmE=) | `78.57% <100.00%> (ø)` | `4.00 <0.00> (ø)` | |
   | [...hdds/scm/container/common/helpers/ExcludeList.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy9zY20vY29udGFpbmVyL2NvbW1vbi9oZWxwZXJzL0V4Y2x1ZGVMaXN0LmphdmE=) | `85.71% <0.00%> (-10.21%)` | `21.00% <0.00%> (-2.00%)` | |
   | [...va/org/apache/hadoop/hdds/utils/db/RDBMetrics.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvZnJhbWV3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy91dGlscy9kYi9SREJNZXRyaWNzLmphdmE=) | `85.71% <0.00%> (-7.15%)` | `13.00% <0.00%> (-1.00%)` | |
   | [...apache/hadoop/hdds/scm/block/BlockManagerImpl.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvc2VydmVyLXNjbS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hkZHMvc2NtL2Jsb2NrL0Jsb2NrTWFuYWdlckltcGwuamF2YQ==) | `66.66% <0.00%> (-5.41%)` | `18.00% <0.00%> (ø%)` | |
   | [...che/hadoop/hdds/scm/pipeline/PipelineStateMap.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvc2VydmVyLXNjbS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hkZHMvc2NtL3BpcGVsaW5lL1BpcGVsaW5lU3RhdGVNYXAuamF2YQ==) | `83.04% <0.00%> (-4.10%)` | `45.00% <0.00%> (-3.00%)` | |
   | [...java/org/apache/hadoop/hdds/utils/db/RDBTable.java](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree#diff-aGFkb29wLWhkZHMvZnJhbWV3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvaGRkcy91dGlscy9kYi9SREJUYWJsZS5qYXZh) | `58.66% <0.00%> (-2.67%)` | `19.00% <0.00%> (-1.00%)` | |
   | ... and [19 more](https://codecov.io/gh/apache/hadoop-ozone/pull/1079/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=footer). Last update [348f7c6...53cbbd8](https://codecov.io/gh/apache/hadoop-ozone/pull/1079?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [hadoop-ozone] ChenSammi edited a comment on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi edited a comment on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-645199042


   Thanks @adoroszlai  for review the code.  A new patch uploaded addressing the comments. 


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

[GitHub] [hadoop-ozone] ChenSammi commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-646016536


   @adoroszlai thanks for the review.  Is it a +1?  Should I wait for @bshashikant 's review and feedback before commit?  


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

[GitHub] [hadoop-ozone] bshashikant commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-646050372


   Thanks @ChenSammi for the contribution and @adoroszlai for the review. I have committed 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

[GitHub] [hadoop-ozone] ChenSammi commented on a change in pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#discussion_r441326124



##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestKeyInputStream.java
##########
@@ -64,25 +71,38 @@
   private static String volumeName;
   private static String bucketName;
   private static String keyString;
+  private static ChunkLayoutTestInfo chunkLayout;
+
+  @Parameterized.Parameters
+  public static Collection<Object[]> layouts() {
+    return Arrays.asList(new Object[][] {
+        {ChunkLayoutTestInfo.FILE_PER_CHUNK},
+        {ChunkLayoutTestInfo.FILE_PER_BLOCK}
+    });
+  }
 
+  public TestKeyInputStream(ChunkLayoutTestInfo layout) {
+    this.chunkLayout = layout;
+  }
   /**
    * Create a MiniDFSCluster for testing.
    * <p>
    * Ozone is made active by setting OZONE_ENABLED = true
    *
    * @throws IOException
    */
-  @BeforeClass
-  public static void init() throws Exception {
-    chunkSize = 100;
+  @Before
+  public void init() throws Exception {
+    chunkSize = 1024 * 1024 * 4;

Review comment:
       I will try to reduce the chunkSize. One limitation here is chunkSize cann't be less than the mininum bytes-per-checksume value which is 256KB, otherwise the TestKeyInputStream will always succeed even without this patch. 




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

[GitHub] [hadoop-ozone] ChenSammi commented on pull request #1079: HDDS-3802. Incorrect data returned by reading a FILE_PER_CHUNK block.

Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1079:
URL: https://github.com/apache/hadoop-ozone/pull/1079#issuecomment-645199042


   Thanks @adoroszlai  for review the code. 


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