Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-11267

Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 2.9.0, 3.0.0-alpha2
    • Component/s: None
    • Labels:
      None

      Description

      In the abstract class Storage, storageDirs is a protected variable and all its derived classes like NNStorage, JNStorage, DataStorage.. are iterating over this non-thread safe variable without any proper locks. Any parallel modification operation like add or remove volume can mutate the backing storageDirs list and any iterators on this list around the same time can face ConcurrentModificationException. It would be good to make the variable private and restrict the access via getters and setters. Any thread safe restriction need to be done can then be placed on the parent class only.

      Also, NNStorage redefines parent class Storage's storageDirs, making it inconsistent with other derived classes. Would be cleaner if NNStorage can avoid re-defining it locally.

      1. HDFS-11267.01.patch
        10 kB
        Manoj Govindassamy
      2. HDFS-11267.02.patch
        10 kB
        Manoj Govindassamy
      3. HDFS-11267.03.patch
        9 kB
        Lei (Eddy) Xu

        Activity

        Hide
        manojg Manoj Govindassamy added a comment -

        The fix for HDFS-11251 (ConcurrentModificationException during DataNode#refreshVolumes) making the Storage#storageDirs a CopyOnWriteArrayList is needed for this bug. On top of HDFS-11251 fix, we need more clean up patches.

        Show
        manojg Manoj Govindassamy added a comment - The fix for HDFS-11251 (ConcurrentModificationException during DataNode#refreshVolumes) making the Storage#storageDirs a CopyOnWriteArrayList is needed for this bug. On top of HDFS-11251 fix, we need more clean up patches.
        Hide
        manojg Manoj Govindassamy added a comment -

        Attaching v01 patch to address the following:

        1. Avoid storageDirs getting re-defined in NNStorage. The redefinition is not needed as HDFS-11251 is anyway making the parent member variable stroageDir a CopyOnWriteArrayList
        2. Make Storage#storageDirs a private member and expose get() method for the same.
        3. Clean up all direct accessors of storageDirs with getStorageDirs() method

        Lei (Eddy) Xu, can you please take a look at the patch ?

        Show
        manojg Manoj Govindassamy added a comment - Attaching v01 patch to address the following: 1. Avoid storageDirs getting re-defined in NNStorage. The redefinition is not needed as HDFS-11251 is anyway making the parent member variable stroageDir a CopyOnWriteArrayList 2. Make Storage#storageDirs a private member and expose get() method for the same. 3. Clean up all direct accessors of storageDirs with getStorageDirs() method Lei (Eddy) Xu , can you please take a look at the patch ?
        Hide
        eddyxu Lei (Eddy) Xu added a comment -

        Hi, Manoj Govindassamy

        Thanks a lot for cleaning the code.

        I have a few questions remained:

        protected List<StorageDirectory> storageDirs = new ArrayList<StorageDirectory>();
        

        Can we change it to private and adds the public getStorageDirs()? Adding new ArrayList<>() to constructor makes duplicated code. Btw, as for JDK 7, the second template parameter can be removed:

        protected List<StorageDirectory> storageDirs = new ArrayList<>();
        
        • Regarding thread-safe iteration, the proposed patch can protected get/set the reference of the StorageDir collection. But it might be difficult to protect iterating the collection.
        • Would using ArrayList() in NNStorage weaken the concurrency protection of NNStorage?

        Thanks!

        Show
        eddyxu Lei (Eddy) Xu added a comment - Hi, Manoj Govindassamy Thanks a lot for cleaning the code. I have a few questions remained: protected List<StorageDirectory> storageDirs = new ArrayList<StorageDirectory>(); Can we change it to private and adds the public getStorageDirs() ? Adding new ArrayList<>() to constructor makes duplicated code. Btw, as for JDK 7, the second template parameter can be removed: protected List<StorageDirectory> storageDirs = new ArrayList<>(); Regarding thread-safe iteration, the proposed patch can protected get/set the reference of the StorageDir collection. But it might be difficult to protect iterating the collection. Would using ArrayList() in NNStorage weaken the concurrency protection of NNStorage ? Thanks!
        Hide
        manojg Manoj Govindassamy added a comment - - edited

        Thanks for the review Lei (Eddy) Xu.

        1. Protected vs Private:
        v01 patch already converted the "protected List" to a "private List", initialized it only in the constructor and added getStorageDirs(). Yes, the second template parameter in the constructor is not needed. Removing it.

        index 1f03fc27cd7c6ea9a0f65d8f0417d581172796ba..2cca60e25bc7cc343bb492bbf464fbc73ba21c23 100644
        --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
        +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
        @@ -123,7 +123,7 @@
             public boolean isOfType(StorageDirType type);
           }
           
        -  protected List<StorageDirectory> storageDirs = new ArrayList<StorageDirectory>();
        +  private List<StorageDirectory> storageDirs;
        ..
        ..
        +
        +  public List<StorageDirectory> getStorageDirs() {
        +    return storageDirs;
        +  }
        +
        

        2. Thread Safety:
        The idea is to use CopyonWriteArrayList in the base Storage class for storageDirs. The proposed patch is missing the CopyOnWriteArrayList as I assumed HDFS-11251 will bring in that fix. But, thinking more about it, its better to make the fix for this issue a self contained one and so will amend the patch with storageDirs initialized with CopyOnWriteArrayList.

        After storageDirs is made a CopyOnWriteArrayList, the iterator will use a snapshot of the underlying list and the iterator will never throw a ConcurrentModificationException. There can be parallel addition and removal of storageDirs and still other threads can iterate over the storageDirs without facing any issues (because, they are iterating on the point in time copy of List).

        So after CopyOnWriteArrayList change, it will be thread safe.

        3. NNStorage concurrency
        Since we are making the base Storage class storageDirs a CopyOnWriteArrayList, there will be no weakening of concurrency protection for NNStorage.

        Show
        manojg Manoj Govindassamy added a comment - - edited Thanks for the review Lei (Eddy) Xu . 1. Protected vs Private: v01 patch already converted the "protected List" to a "private List", initialized it only in the constructor and added getStorageDirs(). Yes, the second template parameter in the constructor is not needed. Removing it. index 1f03fc27cd7c6ea9a0f65d8f0417d581172796ba..2cca60e25bc7cc343bb492bbf464fbc73ba21c23 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java @@ -123,7 +123,7 @@ public boolean isOfType(StorageDirType type); } - protected List<StorageDirectory> storageDirs = new ArrayList<StorageDirectory>(); + private List<StorageDirectory> storageDirs; .. .. + + public List<StorageDirectory> getStorageDirs() { + return storageDirs; + } + 2. Thread Safety: The idea is to use CopyonWriteArrayList in the base Storage class for storageDirs. The proposed patch is missing the CopyOnWriteArrayList as I assumed HDFS-11251 will bring in that fix. But, thinking more about it, its better to make the fix for this issue a self contained one and so will amend the patch with storageDirs initialized with CopyOnWriteArrayList. After storageDirs is made a CopyOnWriteArrayList , the iterator will use a snapshot of the underlying list and the iterator will never throw a ConcurrentModificationException . There can be parallel addition and removal of storageDirs and still other threads can iterate over the storageDirs without facing any issues (because, they are iterating on the point in time copy of List). So after CopyOnWriteArrayList change, it will be thread safe. 3. NNStorage concurrency Since we are making the base Storage class storageDirs a CopyOnWriteArrayList, there will be no weakening of concurrency protection for NNStorage.
        Hide
        manojg Manoj Govindassamy added a comment -

        Attached v02 patch to address previous review comments. Lei (Eddy) Xu, can you please take a look ?

        Show
        manojg Manoj Govindassamy added a comment - Attached v02 patch to address previous review comments. Lei (Eddy) Xu , can you please take a look ?
        Hide
        linyiqun Yiqun Lin added a comment -

        The latest patch LGTM, +1. Thanks Manoj Govindassamy for the follow-up work of HDFS-11251.

        Show
        linyiqun Yiqun Lin added a comment - The latest patch LGTM, +1. Thanks Manoj Govindassamy for the follow-up work of HDFS-11251 .
        Hide
        manojg Manoj Govindassamy added a comment -

        Thanks for the review Yiqun Lin.

        Show
        manojg Manoj Govindassamy added a comment - Thanks for the review Yiqun Lin .
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        -1 patch 0m 5s HDFS-11267 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



        Subsystem Report/Notes
        JIRA Issue HDFS-11267
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12844975/HDFS-11267.02.patch
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17983/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 5s HDFS-11267 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HDFS-11267 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12844975/HDFS-11267.02.patch Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17983/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11055 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11055/)
        HDFS-11267. Avoid redefinition of storageDirs in NNStorage and cleanup (lei: rev a4f66655ec22ca8c960f971f2b0cdafbd3430ad7)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/StorageAdapter.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11055 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11055/ ) HDFS-11267 . Avoid redefinition of storageDirs in NNStorage and cleanup (lei: rev a4f66655ec22ca8c960f971f2b0cdafbd3430ad7) (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/StorageAdapter.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
        Hide
        eddyxu Lei (Eddy) Xu added a comment -

        Attach a new patch to address the conflicts with HDFS-11251.

        Show
        eddyxu Lei (Eddy) Xu added a comment - Attach a new patch to address the conflicts with HDFS-11251 .
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 0s Docker mode activated.
        -1 patch 0m 4s HDFS-11267 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



        Subsystem Report/Notes
        JIRA Issue HDFS-11267
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845033/HDFS-11267.03.patch
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17984/console
        Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 4s HDFS-11267 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HDFS-11267 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845033/HDFS-11267.03.patch Console output https://builds.apache.org/job/PreCommit-HDFS-Build/17984/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        eddyxu Lei (Eddy) Xu added a comment -

        +1 for the 02 patch that Manoj Govindassamy submitted. Committed to trunk and branch-2. Thanks for the good work Manoj Govindassamy and reviews from Yiqun Lin.

        My bad, the patch was accidentally committed to trunk and branch-2 without running jenkins. However, the patch is relatively safe and has passed the review from Yiqun Lin as well. If there is any issue, let's file a follow up JIRA for it.

        Thanks again.

        Show
        eddyxu Lei (Eddy) Xu added a comment - +1 for the 02 patch that Manoj Govindassamy submitted. Committed to trunk and branch-2. Thanks for the good work Manoj Govindassamy and reviews from Yiqun Lin . My bad, the patch was accidentally committed to trunk and branch-2 without running jenkins. However, the patch is relatively safe and has passed the review from Yiqun Lin as well. If there is any issue, let's file a follow up JIRA for it. Thanks again.

          People

          • Assignee:
            manojg Manoj Govindassamy
            Reporter:
            manojg Manoj Govindassamy
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development