Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      The DatanodeID has a single field for the IP address, for HDFS-3146 we need to extend it to support multiple addresses.

      1. hdfs-3216.txt
        19 kB
        Eli Collins
      2. hdfs-3216.txt
        22 kB
        Eli Collins
      3. hdfs-3216.txt
        38 kB
        Eli Collins
      4. hdfs-3216.txt
        38 kB
        Eli Collins

        Activity

        Hide
        Eli Collins added a comment -

        Patch attached.

        Show
        Eli Collins added a comment - Patch attached.
        Hide
        Todd Lipcon added a comment -

        Should we deprecate this function? Or do we need some concept of the "canonical"/"main" IP address? If the latter, we should explain this in the javadoc of this function.

           public String getIpAddr() {
           -    return ipAddr;
           +    return ipAddrs[0];
           +  }
        

        • is it ever valid to construct a DatanodeID with no IP addresses? If not we should add a Preconditions check or at least an assert on the length of the ipAddrs array in the constructor and the setter

        +    return new DatanodeID(ipAddrs.toArray(new String[ipAddrs.size()]) , dn.getHostName(), dn.getStorageID(),
                 dn.getXferPort(), dn.getInfoPort(), dn.getIpcPort());
        

        Can you re-wrap this to 80chars?


        • Is the code change in JsonUtil covered by TestJsonUtil? (are you sure that the cast to String[] is right?)
        • in some of the tests, it's filling in hostnames instead of IPs for the ipAddrs field. Is that right, or do we expect that it will always be resolved IPs? The dual nature makes me nervous.
        Show
        Todd Lipcon added a comment - Should we deprecate this function? Or do we need some concept of the "canonical"/"main" IP address? If the latter, we should explain this in the javadoc of this function. public String getIpAddr() { - return ipAddr; + return ipAddrs[0]; + } is it ever valid to construct a DatanodeID with no IP addresses? If not we should add a Preconditions check or at least an assert on the length of the ipAddrs array in the constructor and the setter + return new DatanodeID(ipAddrs.toArray( new String [ipAddrs.size()]) , dn.getHostName(), dn.getStorageID(), dn.getXferPort(), dn.getInfoPort(), dn.getIpcPort()); Can you re-wrap this to 80chars? Is the code change in JsonUtil covered by TestJsonUtil? (are you sure that the cast to String[] is right?) in some of the tests, it's filling in hostnames instead of IPs for the ipAddrs field. Is that right, or do we expect that it will always be resolved IPs? The dual nature makes me nervous.
        Hide
        Eli Collins added a comment -

        #1 We still need the notion of canonical IP, mostly for cases that don't care about multiple IP addresses. Updated the javadoc.
        #2 Yes, when reading/writing DatanodeInfos to/from streams (same as before when creating a DatanodeID w/o a name)
        #3 Done
        #4 It's tested by TestWebHdfsFileSystemContract#testGetFileBlockLocations. It currently only tests the first IP vs the whole array since LocatedBlock (HDFS) is multi-IP aware but BlockLocation (common) is not. Fixed the cast, now casts to String and serializes/deserializes the IPs, the test does check this (was failing now passes).
        #5 We expect this field to always contain IPs, see HDFS-3144, we no longer overload this field. Filed HDFS-3230 to cleanup the tests, will do it on trunk since its independent of this change.

        Show
        Eli Collins added a comment - #1 We still need the notion of canonical IP, mostly for cases that don't care about multiple IP addresses. Updated the javadoc. #2 Yes, when reading/writing DatanodeInfos to/from streams (same as before when creating a DatanodeID w/o a name) #3 Done #4 It's tested by TestWebHdfsFileSystemContract#testGetFileBlockLocations. It currently only tests the first IP vs the whole array since LocatedBlock (HDFS) is multi-IP aware but BlockLocation (common) is not. Fixed the cast, now casts to String and serializes/deserializes the IPs, the test does check this (was failing now passes). #5 We expect this field to always contain IPs, see HDFS-3144 , we no longer overload this field. Filed HDFS-3230 to cleanup the tests, will do it on trunk since its independent of this change.
        Hide
        Todd Lipcon added a comment -

        #2 Yes, when reading/writing DatanodeInfos to/from streams (same as before when creating a DatanodeID w/o a name)

        When do we read/write DatanodeInfo from streams, now that we are pb-ified? i.e is the writable interface even used anymore?


        +   * Return the canonical IP address for this DatanodeID. Not all uses
        +   * of DatanodeID are multi-IP aware, or would multiple IPs, therefore
        +   * we use the first address as the canonical one.
        

        ENOTASENTENCE


        #1 We still need the notion of canonical IP, mostly for cases that don't care about multiple IP addresses. Updated the javadoc.

        How is it ensured that the "canonical" IP is kept consistent across DN restarts, for example? It's just whichever one is listed first in the DN-side configuration?

        Fixed the cast, now casts to String and serializes/deserializes the IPs, the test does check this (was failing now passes).

        That's a little strange, to serialize it into a comma-separated list inside JSON. It's not possible to get Jackson to serialize it as a proper JSON array? Perhaps using a List<String> inside the map?

        Show
        Todd Lipcon added a comment - #2 Yes, when reading/writing DatanodeInfos to/from streams (same as before when creating a DatanodeID w/o a name) When do we read/write DatanodeInfo from streams, now that we are pb-ified? i.e is the writable interface even used anymore? + * Return the canonical IP address for this DatanodeID. Not all uses + * of DatanodeID are multi-IP aware, or would multiple IPs, therefore + * we use the first address as the canonical one. ENOTASENTENCE #1 We still need the notion of canonical IP, mostly for cases that don't care about multiple IP addresses. Updated the javadoc. How is it ensured that the "canonical" IP is kept consistent across DN restarts, for example? It's just whichever one is listed first in the DN-side configuration? Fixed the cast, now casts to String and serializes/deserializes the IPs, the test does check this (was failing now passes). That's a little strange, to serialize it into a comma-separated list inside JSON. It's not possible to get Jackson to serialize it as a proper JSON array? Perhaps using a List<String> inside the map?
        Hide
        Eli Collins added a comment -

        Updated patch attach addresses feedback. Will comment wrt how multiple IPs per DatanodeID work in HDFS-3146 since the discussion is relevant there. This one will just cover the plumbing.

        Show
        Eli Collins added a comment - Updated patch attach addresses feedback. Will comment wrt how multiple IPs per DatanodeID work in HDFS-3146 since the discussion is relevant there. This one will just cover the plumbing.
        Hide
        Eli Collins added a comment -

        Forgot to mention that in this patch only one IP is still ever used, ie just updating the ipAddr field in DatanodeIDProto to be repeated and plumbing that through.

        Show
        Eli Collins added a comment - Forgot to mention that in this patch only one IP is still ever used, ie just updating the ipAddr field in DatanodeIDProto to be repeated and plumbing that through.
        Hide
        Eli Collins added a comment -

        Re-based on hdfs-3416.

        Show
        Eli Collins added a comment - Re-based on hdfs-3416.

          People

          • Assignee:
            Eli Collins
            Reporter:
            Eli Collins
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development