A preliminary patch to get any thoughts on the approach.

This just serializes multiple registrations with a yield between retry attempts. We can probably avoid the serialization for all registrations after the first one.

Updated the patch to avoid serialization except for the first time a given Datanode registers.

All subsequent registrations behave as before.

An issue with relying on the NameNode handshake to assign the UUID is that there is a lengthy period during DataNode initialization where the DatanodeUuid is still NULL. In particular, when the FsDatasetSpi is created in initStorage(), the DatanodeUuid is NULL. The DatanodeUuid is not set afterwards until bpRegistrationSucceeded().

My team is porting our FsDataset plugin from Hadoop 1.x to Hadoop 2.x and this behavior as changed (in Hadoop 1.x the DatanodeUuid was available when our FsDataset plugin was initialized).

Due to this condition, I vote for option 2 "The datanode assigns itself a UUID on initialization".

It seems that option 2 is simpler. Is there any drawback?

No drawback, unless we want to let NameNodes manage the UUID assignment.

An issue with relying on the NameNode handshake to assign the UUID is that there is a lengthy period during DataNode initialization where the DatanodeUuid is still NULL. In particular, when the FsDatasetSpi is created in initStorage(), the DatanodeUuid is NULL. The DatanodeUuid is not set afterwards until bpRegistrationSucceeded().

Eric Sirianni, I believe this is not very different from today where the storageID remains unassigned until after the first handshake.

Updated patch to generate the id on the datanode.

+1 patch looks good.

Thanks Nicholas. I committed this to branch HDFS-2832.

Eric Sirianni, I believe this is not very different from today where the storageID remains unassigned until after the first handshake.

Arpit Agarwal - true, your change to synchronize the registrations did not create the issue I was seeing - I haven't fully root caused, but that seems to have been caused by the changes to DataNode initialization ordering for namespace federation.

I chose to comment on this particular JIRA since my observations about NULL UUIDs were relevant in favoring "option 2". Thanks for making the change!

Arpit - I think your patch causes a compile error:

[ERROR] hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPOfferService.java:[337,34] error: unreported exception IOException; must be caught or declared to be thrown

Seems to be caused by this change:

+  DatanodeRegistration createBPRegistration(NamespaceInfo nsInfo)
+      throws IOException {

I'm sorry, the error was only triggered on a clean build.

Thanks for the catch Eric, can you please review the simple fix?

+1 patch looks good. I made the same change in my environment and the code compiles successfully.

Thanks Eric, I committed the fix to branch HDFS-2832.

Arpit - thanks for the change. The current point of UUID initialization still leaves a lengthy window during DataNode startup where UUID is null:

  private void connectToNNAndHandshake() throws IOException {

    // ...

// FsDatasetInitialization happens as a side effect of this call    
    bpos.verifyAndSetNamespaceInfo(nsInfo);
    
// Datanode.checkDatanodeUuid() happens as a side effect of this call
    register();
  }

Would it be possible to simply initialize the UUID immediately in the DataStorage constructor and then overwrite it from the actual properties file later via setFieldsFromProperties() (in the normal case when this is not the first time the DataNode has started).

(I realize we've moved a bit now from the original JIRA, but it seems reasonable to take care of this as part of this change)

Eric, let's discuss this on a separate Jira that is not a sub-task of HDFS-2832. Please feel free to file one.

Actually, rather than having a potentially temporary UUID in the DataStorage object, (as I suggested above), a better solution would be to initialize the UUID in DataNode.initStorage():

  private void initStorage(final NamespaceInfo nsInfo) throws IOException {
    // ...

      final String bpid = nsInfo.getBlockPoolID();
      //read storage info, lock data dirs and transition fs state if necessary
      storage.recoverTransitionRead(this, bpid, nsInfo, dataDirs, startOpt);
      
      // SUGGESTED NEW PLACE TO CHECK DATANODE UUID
      checkDatanodeUuid();

    // ...
  }

Arpit - is there any issue with moving the call to checkDatanodeUuid() to initStorage() as suggested above?

OK - I've filed HDFS-5454 for the aforementioned DataNode UUID initialization issue.

> Prior to the heterogeneous storage feature, each Datanode had a single storage ID which was generated by the Namenode on first registration.

Is that right? storageIDs were generated by DataNodes, not NameNodes, unless I missed the change.

storageID was generated by the NameNode in DatanodeManager#registerDatanode

From trunk

      // this is a new datanode serving a new data storage
      if ("".equals(nodeReg.getStorageID())) {
        // this data storage has never been registered
        // it is either empty or was created by pre-storageID version of DFS
        nodeReg.setStorageID(newStorageID());

Ah yes, this is a legacy and pretty confusing part of code, which remained from the times when DNs could not generate their own storageIDs. Now that they do, see DataStorage.createStorageID(), this code path is just never triggered, because nodeReg.getStorageID() is never empty.
So in practice NameNode never generates storageIDs for DNs.

Thanks for the correction, you're right.