[ 
https://issues.apache.org/jira/browse/HDFS-7575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14274124#comment-14274124
 ] 

Colin Patrick McCabe commented on HDFS-7575:
--------------------------------------------

[~daryn]: I think a new layout version makes sense here.  Basically we are 
going from a layout where the storageID might not have been unique, to one 
where it is.  This is a change in the VERSION file.  It's nice to have the same 
guarantees that we usually do (that if the ugprade fails, you can roll back via 
the {{previous}} directory, and so forth.)  We could probably be more "clever" 
here and optimize this so we didn't have to hardlink the block files, but the 
upgrade path is already a little too "clever" and I think this is fine.

Rather than calling the new layout version UPGRADE_GENERATES_STORAGE_IDS, how 
about calling it something like UNIQUE_STORAGE_IDS or 
GUARANTEED_UNIQUE_STORAGE_IDS?  That describes what the new layout is, rather 
than what the process of upgrading is, consistent with our other layout version 
descriptions.

{code}
110    ... = new ClusterVerifier() {
111           @Override
112           public void verifyClusterPostUpgrade(MiniDFSCluster cluster) 
throws IOException {
113             // Verify that a GUID-based storage ID was generated.
114             final String bpid = cluster.getNamesystem().getBlockPoolId();
115             StorageReport[] reports =
116                 
cluster.getDataNodes().get(0).getFSDataset().getStorageReports(bpid);
117             assertThat(reports.length, is(1));
118             final String storageID = reports[0].getStorage().getStorageID();
119             assertTrue(DatanodeStorage.isValidStorageId(storageID));
120           }
{code}
It seems like this exact code appears in 3 different tests.  We should just 
make this Verifier a static object that's created once in the test or something?

+1 once these are addressed.  [~daryn], please take a look if you can... we'd 
really like to fix this one.  Thanks, guys

> NameNode not handling heartbeats properly after HDFS-2832
> ---------------------------------------------------------
>
>                 Key: HDFS-7575
>                 URL: https://issues.apache.org/jira/browse/HDFS-7575
>             Project: Hadoop HDFS
>          Issue Type: Bug
>    Affects Versions: 2.4.0, 2.5.0, 2.6.0
>            Reporter: Lars Francke
>            Assignee: Arpit Agarwal
>            Priority: Critical
>         Attachments: HDFS-7575.01.patch, HDFS-7575.02.patch, 
> HDFS-7575.03.binary.patch, HDFS-7575.03.patch, 
> testUpgrade22via24GeneratesStorageIDs.tgz, 
> testUpgradeFrom22GeneratesStorageIDs.tgz, 
> testUpgradeFrom24PreservesStorageId.tgz
>
>
> Before HDFS-2832 each DataNode would have a unique storageId which included 
> its IP address. Since HDFS-2832 the DataNodes have a unique storageId per 
> storage directory which is just a random UUID.
> They send reports per storage directory in their heartbeats. This heartbeat 
> is processed on the NameNode in the 
> {{DatanodeDescriptor#updateHeartbeatState}} method. Pre HDFS-2832 this would 
> just store the information per Datanode. After the patch though each DataNode 
> can have multiple different storages so it's stored in a map keyed by the 
> storage Id.
> This works fine for all clusters that have been installed post HDFS-2832 as 
> they get a UUID for their storage Id. So a DN with 8 drives has a map with 8 
> different keys. On each Heartbeat the Map is searched and updated 
> ({{DatanodeStorageInfo storage = storageMap.get(s.getStorageID());}}):
> {code:title=DatanodeStorageInfo}
>   void updateState(StorageReport r) {
>     capacity = r.getCapacity();
>     dfsUsed = r.getDfsUsed();
>     remaining = r.getRemaining();
>     blockPoolUsed = r.getBlockPoolUsed();
>   }
> {code}
> On clusters that were upgraded from a pre HDFS-2832 version though the 
> storage Id has not been rewritten (at least not on the four clusters I 
> checked) so each directory will have the exact same storageId. That means 
> there'll be only a single entry in the {{storageMap}} and it'll be 
> overwritten by a random {{StorageReport}} from the DataNode. This can be seen 
> in the {{updateState}} method above. This just assigns the capacity from the 
> received report, instead it should probably sum it up per received heartbeat.
> The Balancer seems to be one of the only things that actually uses this 
> information so it now considers the utilization of a random drive per 
> DataNode for balancing purposes.
> Things get even worse when a drive has been added or replaced as this will 
> now get a new storage Id so there'll be two entries in the storageMap. As new 
> drives are usually empty it skewes the balancers decision in a way that this 
> node will never be considered over-utilized.
> Another problem is that old StorageReports are never removed from the 
> storageMap. So if I replace a drive and it gets a new storage Id the old one 
> will still be in place and used for all calculations by the Balancer until a 
> restart of the NameNode.
> I can try providing a patch that does the following:
> * Instead of using a Map I could just store the array we receive or instead 
> of storing an array sum up the values for reports with the same Id
> * On each heartbeat clear the map (so we know we have up to date information)
> Does that sound sensible?



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to