[
https://issues.apache.org/jira/browse/HDFS-10759?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15813585#comment-15813585
]
Andrew Wang commented on HDFS-10759:
------------------------------------
Thanks for the rev Ewan. Overall this looks really good, just nitty review
comments. Since this is a big patch with a lot of mechanical changes, I'll try
to review promptly to reduce rebase overhead. Some review comments:
* It looks like checkstyle is unhappy, I think your IDE is set to wider than 80
chars line width, some other issues too.
* It'd be good to have a test that the default value of the proto enum is
CONTIGUOUS as expected, per Jing's concern.
* INodeFile#BLOCK_TYPE_MASK_CONTIGUOUS is unused
* BLOCK_ID_MASK_STRIPED could use a comment, I double taked initially when I
saw blockType was being compared against a variable named MASK, and it was the
same value as BLOCK_ID_MASK.
* BlockType could use some more unit tests with filled in lower values.
> Change fsimage bool isStriped from boolean to an enum
> -----------------------------------------------------
>
> Key: HDFS-10759
> URL: https://issues.apache.org/jira/browse/HDFS-10759
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: hdfs
> Affects Versions: 3.0.0-alpha1, 3.0.0-beta1, 3.0.0-alpha2
> Reporter: Ewan Higgs
> Labels: hdfs-ec-3.0-must-do
> Attachments: HDFS-10759.0001.patch, HDFS-10759.0002.patch,
> HDFS-10759.0003.patch
>
>
> The new erasure coding project has updated the protocol for fsimage such that
> the {{INodeFile}} has a boolean '{{isStriped}}'. I think this is better as an
> enum or integer since a boolean precludes any future block types.
> For example:
> {code}
> enum BlockType {
> CONTIGUOUS = 0,
> STRIPED = 1,
> }
> {code}
> We can also make this more robust to future changes where there are different
> block types supported in a staged rollout. Here, we would use
> {{UNKNOWN_BLOCK_TYPE}} as the first value since this is the default value.
> See
> [here|http://androiddevblog.com/protocol-buffers-pitfall-adding-enum-values/]
> for more discussion.
> {code}
> enum BlockType {
> UNKNOWN_BLOCK_TYPE = 0,
> CONTIGUOUS = 1,
> STRIPED = 2,
> }
> {code}
> But I'm not convinced this is necessary since there are other enums that
> don't use this approach.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
