[
https://issues.apache.org/jira/browse/HDFS-14172?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16745877#comment-16745877
]
Xiang Li edited comment on HDFS-14172 at 1/18/19 8:58 AM:
----------------------------------------------------------
[~adam.antal], thanks very much for the comments!
1. I found there is a weird logic in the comparator mentioned in your comment:
{code:java}
if (n1 == null) {
return n2 == null ? 0 : -1;
} else if (n2 == null) {
return -1;
} else {
return n1.ordinal() - n2.ordinal();
}
{code}
Given the logic above and let's say n is not null, then
compare(null, n) = -1 and compare (n, null) = -1.
The results above seems break the rule of comparator/comparable, that is:
compare (n1, n2) = - compare (n2, n1). Do you have any context or could recall
the discussion log before, about why it is designed like that? (Any idea here?
[~jingzhao] [~wheat9], found you in the git log)
2. Regarding:
{quote}In this function the sections are ordered this way, and they're aware
that the SectionName can be null. I think we should follow the same pattern
later.
{quote}
Maybe by adding a default value and not returning null, we could simplify the
code as (suppose item 1 above is a typo/bug)
{code:java}
Collections.sort(sections, new Comparator<FileSummary.Section>() {
@Override
public int compare(FileSummary.Section s1, FileSummary.Section s2) {
SectionName n1 = SectionName.fromString(s1.getName());
SectionName n2 = SectionName.fromString(s2.getName());
return n1.ordinal() - n2.ordinal(); // do not need to care about null here
}
});
{code}
3. Regarding:
{quote}Maybe at the end if there was a section which is not null, but wasn't
handled in the switch, we would throw an IOException
{quote}
Strongly agree. Currently, the code only records a warn here, which is a little
weak. But if the code does not catch the IOException, the program will exit. We
might need to provide the compatibility to let the old code could handle a
fsimage with new section names. What about your opinion?
was (Author: water):
[~adam.antal], thanks very much for the comments!
1. I found there is a weird logic in the comparator mentioned in your comment:
{code:java}
if (n1 == null) {
return n2 == null ? 0 : -1;
} else if (n2 == null) {
return -1;
} else {
return n1.ordinal() - n2.ordinal();
}
{code}
Given the logic above and let's say n is not null, then
compare(null, n) = -1 and compare (n, null) = -1.
The results above seems break the rule of comparator/comparable, that is:
compare (n1, n2) = - compare (n2, n1). Do you have any context or could recall
the discussion log before, about why it is designed like that? (Any idea here?
[~jingzhao] [~wheat9], found you in the git log)
2. Regarding:
{quote}In this function the sections are ordered this way, and they're aware
that the SectionName can be null. I think we should follow the same pattern
later.
{quote}
Maybe by adding a default value and not returning null, we could simplify the
code as
{code:java}
Collections.sort(sections, new Comparator<FileSummary.Section>() {
@Override
public int compare(FileSummary.Section s1, FileSummary.Section s2) {
SectionName n1 = SectionName.fromString(s1.getName());
SectionName n2 = SectionName.fromString(s2.getName());
return n1.ordinal() - n2.ordinal(); // do not need to care about null here
}
});
{code}
3. Regarding:
bq. Maybe at the end if there was a section which is not null, but wasn't
handled in the switch, we would throw an IOException
Strongly agree. Currently, the code only records a warn here, which is a little
weak. But if the code does not catch the IOException, the program will exit. We
might need to provide the compatibility to let the old code could handle a
fsimage with new section names. What about your opinion?
> Return a default SectionName to avoid NPE
> -----------------------------------------
>
> Key: HDFS-14172
> URL: https://issues.apache.org/jira/browse/HDFS-14172
> Project: Hadoop HDFS
> Issue Type: Improvement
> Reporter: Xiang Li
> Assignee: Xiang Li
> Priority: Minor
>
> In FSImageFormatProtobuf.SectionName#fromString(), as follows:
> {code:java}
> public static SectionName fromString(String name) {
> for (SectionName n : values) {
> if (n.name.equals(name))
> return n;
> }
> return null;
> }
> {code}
> When the code meets an unknown section from the fsimage, the function will
> return null. Callers always operates the return value with a "switch" clause,
> like FSImageFormatProtobuf.Loader#loadInternal(), as:
> {code:java}
> switch (SectionName.fromString(n))
> {code}
> NPE will be thrown here.
> For self-protection, shall we add a default section name in the enum of
> SectionName, like "UNKNOWN", to steer clear of NPE?
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
