Details

    • Hadoop Flags:
      Reviewed

      Description

      The perVolumeReplicaMap in FsDatasetImpl.java is not necessary and can be removed. We continue to use the existing volumeMap.

      1. h5648.02.patch
        11 kB
        Arpit Agarwal
      2. h5648.08.patch
        12 kB
        Arpit Agarwal
      3. h5648.09.patch
        12 kB
        Arpit Agarwal
      4. h5648.10.patch
        12 kB
        Arpit Agarwal

        Activity

        Hide
        Arpit Agarwal added a comment -

        Updated patch to follow - this one breaks a few tests.

        Show
        Arpit Agarwal added a comment - Updated patch to follow - this one breaks a few tests.
        Hide
        Arpit Agarwal added a comment -

        Updated patch fixes an unrelated bug exposed by the earlier patch. DatanodeStorage was not overriding Object.equals() and Object.hashCode().

        Show
        Arpit Agarwal added a comment - Updated patch fixes an unrelated bug exposed by the earlier patch. DatanodeStorage was not overriding Object.equals() and Object.hashCode() .
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1 patch looks good.

        A minor comment: the type of volumes.volumes is already List<FsVolumeImpl>. The for loop below should be "for(FsVolumeImpl v : : volumes.volumes)". Then the cast in "((FsVolumeImpl) v).toDatanodeStorage()" is unnecessary.

        +    for (FsVolumeSpi v : volumes.volumes) {
        +      ArrayList<ReplicaInfo> finalizedList = finalized.get(v.getStorageID());
        +      ArrayList<ReplicaInfo> ucList = uc.get(v.getStorageID());
        +      blockReportsMap.put(((FsVolumeImpl) v).toDatanodeStorage(),
        +                          new BlockListAsLongs(finalizedList, ucList));
        +    }
        
        Show
        Tsz Wo Nicholas Sze added a comment - +1 patch looks good. A minor comment: the type of volumes.volumes is already List<FsVolumeImpl>. The for loop below should be "for(FsVolumeImpl v : : volumes.volumes)". Then the cast in "((FsVolumeImpl) v).toDatanodeStorage()" is unnecessary. + for (FsVolumeSpi v : volumes.volumes) { + ArrayList<ReplicaInfo> finalizedList = finalized.get(v.getStorageID()); + ArrayList<ReplicaInfo> ucList = uc.get(v.getStorageID()); + blockReportsMap.put(((FsVolumeImpl) v).toDatanodeStorage(), + new BlockListAsLongs(finalizedList, ucList)); + }
        Hide
        Arpit Agarwal added a comment -

        Thanks for the quick review Nicholas! Updated patch with your feedback attached for reference.

        I will commit this shortly.

        Show
        Arpit Agarwal added a comment - Thanks for the quick review Nicholas! Updated patch with your feedback attached for reference. I will commit this shortly.
        Hide
        Arpit Agarwal added a comment -

        I committed this, thanks again Nicholas.

        Show
        Arpit Agarwal added a comment - I committed this, thanks again Nicholas.

          People

          • Assignee:
            Arpit Agarwal
            Reporter:
            Arpit Agarwal
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development