hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-10999) Introduce separate stats for Replicated and Erasure Coded Blocks apart from the current Aggregated stats
Date Wed, 03 May 2017 02:44:04 GMT

    [ 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: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message