hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinayakumar B (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-11646) Erasure Coder API for encoding and decoding of block group
Date Thu, 05 Mar 2015 09:58:38 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-11646?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14348510#comment-14348510
] 

Vinayakumar B commented on HADOOP-11646:
----------------------------------------

Hi kai,

Here are some of the comments of first look.

1. {code}
+  /**
+   *
+   * @return true if it's missing, otherwise false
+   */
+  public boolean isErased() {
+    return isErased;
+  }{code}
{{isErased}}, Do you mean is block is missing and needs to re-construct?
if Yes, I think better name would be {{isMissed}} itself.


2.
{code}+  @Override
+  public ErasureCodingStep decode(ECBlockGroup blockGroup) {
+    return performDecoding(blockGroup);
+  }
+
+  /**
+   * Perform decoding against a block blockGroup.
+   * @param blockGroup
+   * @return decoding step for caller to do the real work
+   */
+  protected abstract ErasureCodingStep performDecoding(ECBlockGroup blockGroup);{code}

Why there are two separate methods with same params. Why can't just directly IMPLs can implement
{{decode(..)}} instead of implementing {{performDecoding(..)}} ?
Same Q in AbstractErasureEncoder also

3.
For copying from one array to another System.arrayCopy(..) would be better.
{code}
+    int idx = 0;
+    for (int i = 0; i < getNumParityUnits(); i++) {
+      inputBlocks[idx ++] = blockGroup.getParityBlocks()[i];
+    }
+    for (int i = 0; i < getNumDataUnits(); i++) {
+      inputBlocks[idx ++] = blockGroup.getDataBlocks()[i];
+    }{code}
could be done as 
{code}    System.arraycopy(blockGroup.getParityBlocks(), 0, inputBlocks, 0,
        getNumParityUnits());
    System.arraycopy(blockGroup.getDataBlocks(), 0, inputBlocks,
        getNumParityUnits(), getNumDataUnits());{code}
Correct?

4. I think {{protected ECBlock[] getOutputBlocks(ECBlockGroup blockGroup) {}} could be re-written
as below for simplicity.
{code}  protected int getNumErasedBlocks(ECBlockGroup blockGroup) {
    int num = getNumErasedBlocks(blockGroup.getParityBlocks());
    num += getNumErasedBlocks(blockGroup.getDataBlocks());
    return num;
  }{code}


5. Need to track the index of the erasedBlocks separately. Otherwise will throw AIOBException
{code}+    ECBlock[] erasedBlocks = new ECBlock[numErased];
+    for (int i = 0; i < inputBlocks.length; i++) {
+      if (inputBlocks[i].isErased()) {
+        erasedBlocks[i] = inputBlocks[i];
+      }
+    }{code}

6. Rename the patch with HADOOP prefix

> Erasure Coder API for encoding and decoding of block group
> ----------------------------------------------------------
>
>                 Key: HADOOP-11646
>                 URL: https://issues.apache.org/jira/browse/HADOOP-11646
>             Project: Hadoop Common
>          Issue Type: Sub-task
>            Reporter: Kai Zheng
>            Assignee: Kai Zheng
>             Fix For: HDFS-7285
>
>         Attachments: HDFS-7662-v1.patch, HDFS-7662-v2.patch, HDFS-7662-v3.patch
>
>
> This is to define ErasureCoder API for encoding and decoding of BlockGroup. Given a BlockGroup,
ErasureCoder extracts data chunks from the blocks and leverages RawErasureCoder defined in
HDFS-7353 to perform concrete encoding or decoding.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message