[
https://issues.apache.org/jira/browse/HDFS-10999?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15994188#comment-15994188
]
Andrew Wang commented on HDFS-10999:
------------------------------------
Thanks Manoj for taking on this substantial task. The patch is big, so this is
a long review. Sorry if there's overlap with Takanobu's comments.
Naming questions:
I looked at the two new MBeans to get a feel for the APIs. Pasted here for
reference:
{code}
long getUnderReplicatedBlocksStat();
long getCorruptReplicaBlocksStat();
long getMissingReplicaBlocksStat();
long getMissingReplicaOneBlocksStat();
long getReplicaBlocksBytesInFutureStat();
long getPendingDeletionReplicaBlocksStat();
{code}
{code}
long getECLowRedundancyBlocksStat();
long getECCorruptBlocksStat();
long getECMissingBlocksStat();
long getECBlocksBytesInFutureStat();
long getECPendingDeletionBlocksStat();
{code}
I'd like to try and standardize the naming.
ReplicatedBlocksStatsMBean:
* Some weird naming with "ReplicaBlocks" in many names. For example, the
equivalent of {{getCorruptBlocks}} is now called
{{getCorruptReplicaBlocksStat}}. Why not {{getCorruptBlocksStat}} instead? Or
was this meant to be a shortening of {{getCorruptReplicatedBlocksStat}}? This
wasn't obvious to me.
* Should we rename getUnderReplicatedBlocksStat to
{{getLowRedundancyReplicatedBlocksStat}} or similar to standardize with the EC
naming? Since we deprecated {{getUnderReplicatedBlocks}} in favor of
{{getLowRedundancyBlocks}}, it seems odd to bring this same name back here.
ECBlockGroupsStatusMBean:
* the noun is an "EC block group", should we name these e.g.
{{getLowRedundancyECBlockGroupsStat}}, {{getCorruptECBlockGroupsStat}}, etc.?
We could also shorten from "ECBlockGroup" to just "BlockGroup" if you think the
MBean name by itself is sufficient documentation.
* Some replication-related metrics are not split yet, e.g. postponed
misreplicated blocks, excess blocks, timed out pending reconstructions. This
patch is large enough so let's document the gaps in a follow-on JIRA. The
timed-out pending reconstruction metric does seems useful.
Misc:
* Any reason you chose to use LongAccumulator rather than LongAdder everywhere?
* DFSClient and DistributedFileSystem aren't public APIs, so we don't need to
preserve {{getUnderReplicatedBlocksCount}} if these methods are unused. Can
move DFSAdmin over to using the new call.
CorruptReplicasMap:
* Nit: Please put the new increment and decrement functions next to each other
for clarity
* In this block of code, should the decrement be moved inside the {{isEmpty}}
case? Is there a unit test that covers this?
{code}
if (datanodes.remove(datanode) != null) { // remove the replicas
if (datanodes.isEmpty()) {
// remove the block if there is no more corrupted replicas
corruptReplicasMap.remove(blk);
}
decrementBlockStat(blk);
return true;
}
{code}
InvalidateBlocks:
* A lot of the new code is because of the indexing into the new array in the
map and maintaining the separate counts. If we instead add another map, e.g.
{{nodeToBlockGroups}}, we could avoid this.
* If we do the above, to avoid code duplication we can potentially use a merge
iterator (e.g. LazyIteratorChain) to wrap both the replicated blocks and block
group maps / LWHashSets.
* Can we simplify the limiting code on 297 as follows?
{noformat}
List<> polled = blockSet.pollN(remainingLimit);
remainingLimit -= polled.size();
toInvalidate.addAll(polled);
{noformat}
LowRedundancyBlocks
* Rename from StripedBlocks to instead StripedBlockGroups?
* We're duplicating the corruptReplOneBlocks if statement, let's either move
handling {{corruptReplicatedOneBlocks}} out of {{incrementBlockStat}}, or also
increment {{corruptReplOneBlocks}} inside {{incrementBlockStat}}. Same comment
for {{remove}}
Testing:
* How concerning is the TODO in TestNameNodeMetrics?
* Can {{waitForDnMetricValue}} be rewritten to use GenericTestUtils.waitFor?
* I would feel better if there were more unit testing of major modified classes
like InvalidateBlocks, CorruptReplicasMap, LowRedundancyBlocks, to make sure
that the counts remain consistent with different patterns of adds and removes.
I think these classes can be instantiated without a minicluster, thus true unit
tests.
> Introduce separate stats for Replicated and Erasure Coded Blocks apart from
> the current Aggregated stats
> --------------------------------------------------------------------------------------------------------
>
> Key: HDFS-10999
> URL: https://issues.apache.org/jira/browse/HDFS-10999
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: erasure-coding
> Affects Versions: 3.0.0-alpha1
> Reporter: Wei-Chiu Chuang
> Assignee: Manoj Govindassamy
> Labels: hdfs-ec-3.0-nice-to-have, supportability
> Attachments: HDFS-10999.01.patch, HDFS-10999.02.patch
>
>
> Per HDFS-9857, it seems in the Hadoop 3 world, people prefer the more generic
> term "low redundancy" to the old-fashioned "under replicated". But this term
> is still being used in messages in several places, such as web ui, dfsadmin
> and fsck. We should probably change them to avoid confusion.
> File this jira to discuss it.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
