[
https://issues.apache.org/jira/browse/HDFS-11565?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15963466#comment-15963466
]
Wei-Chiu Chuang edited comment on HDFS-11565 at 4/10/17 8:47 PM:
-----------------------------------------------------------------
Hi Andrew! Thanks for updating the patch and I think it's mostly good. I've got
one question though:
The test code
{code}
// Convert proto back to an object and check for equality.
ErasureCodingPolicy convertedPolicy = PBHelperClient
.convertErasureCodingPolicy(proto);
assertEquals("Converted policy not equal", policy, convertedPolicy);
{code}
aserts the two ECP are equal. So I looked at ErasureCodingPolicy#equals too:
{code:title=ErasureCodingPolicy#equals}
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
ErasureCodingPolicy that = (ErasureCodingPolicy) o;
return that.getName().equals(name) &&
that.getCellSize() == cellSize &&
that.getSchema().equals(schema);
}
{code}
It only compares name, cellsize and schema. Should it also compare ECP id?
This is not directly related to your patch, but given that an ECP id is not
guaranteed to map to a unique ErasureCodingPolicy (say a client connects to two
different cluster running different custom ErasureCodingPolicy) it seems
reasonable to add one more check.
was (Author: jojochuang):
Hi Andrew! Thanks for updating the patch and I think it's mostly good. I've got
one question though:
The test code
{code}
// Convert proto back to an object and check for equality.
ErasureCodingPolicy convertedPolicy = PBHelperClient
.convertErasureCodingPolicy(proto);
assertEquals("Converted policy not equal", policy, convertedPolicy);
{code}
aserts the two ECP are equal. So I looked at ErasureCodingPolicy#equals too:
{code:title=ErasureCodingPolicy#equals}
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
ErasureCodingPolicy that = (ErasureCodingPolicy) o;
return that.getName().equals(name) &&
that.getCellSize() == cellSize &&
that.getSchema().equals(schema);
}
{code}
It only compares name, cellsize and schema. Should it also compare ECP id?
> Use compact identifiers for built-in ECPolicies in HdfsFileStatus
> -----------------------------------------------------------------
>
> Key: HDFS-11565
> URL: https://issues.apache.org/jira/browse/HDFS-11565
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: erasure-coding
> Affects Versions: 3.0.0-alpha2
> Reporter: Andrew Wang
> Assignee: Andrew Wang
> Priority: Blocker
> Labels: hdfs-ec-3.0-must-do
> Attachments: HDFS-11565.001.patch, HDFS-11565.002.patch
>
>
> Discussed briefly on HDFS-7337 with Kai Zheng. Quoting our convo:
> {quote}
> From looking at the protos, one other question I had is about the overhead of
> these protos when using the hardcoded policies. There are a bunch of strings
> and ints, which can be kind of heavy since they're added to each
> HdfsFileStatus. Should we make the built-in ones identified by purely an ID,
> with these fully specified protos used for the pluggable policies?
> {quote}
> {quote}
> Sounds like this could be considered separately because, either built-in
> policies or plugged-in polices, the full meta info is maintained either by
> the codes or in the fsimage persisted, so identifying them by purely an ID
> should works fine. If agree, we could refactor the codes you mentioned above
> separately.
> {quote}
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
