Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-11659

o.a.h.fs.FileSystem.Cache#remove should use a single hash map lookup

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The method looks up the same key in the same hash map potentially 3 times

      if (map.containsKey(key) && fs == map.get(key)) {
        map.remove(key)
      

      Instead it could do a single lookup

      FileSystem cachedFs = map.remove(key);
      

      and then test cachedFs == fs or something else.

      1. HADOOP-11659.patch
        0.8 kB
        Brahma Reddy Battula
      2. HADOOP-11659-002.patch
        1.0 kB
        Brahma Reddy Battula
      3. HADOOP-11659-003.patch
        1.0 kB
        Brahma Reddy Battula
      4. HADDOOP-11659-004.patch
        1 kB
        Brahma Reddy Battula

        Activity

        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Attached the patch and did not return testcases but executed effected testcases for regression and all are passing..

        Show
        brahmareddy Brahma Reddy Battula added a comment - Attached the patch and did not return testcases but executed effected testcases for regression and all are passing..
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12702588/HADOOP-11659.patch
        against trunk revision ed70fa1.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5846//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5846//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12702588/HADOOP-11659.patch against trunk revision ed70fa1. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5846//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5846//console This message is automatically generated.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Hi Brahma Reddy Battula, thanks for the patch.
        The patch will change the behavior if map.contains(key) and fs != map.get(key). I'm thinking the method still needs double lookups as follows:

            synchronized void remove(Key key, FileSystem fs) {
              assert fs != null;
              if (map.get(key) == fs) {
                map.remove(key);
                toAutoClose.remove(key);
              }
            }
        

        By the way, the method has two callers. One caller

        FileSystem.Cache#closeAll
                //remove from cache
                remove(key, fs);
        

        can be changed as

                //remove from cache
                map.remove(key);
                toAutoClose.remove(key);
        

        since map.contains(key) and fs == map.get(key) are guaranteed there. That way we can remove the key from the map by single lookup.

        Show
        ajisakaa Akira Ajisaka added a comment - Hi Brahma Reddy Battula , thanks for the patch. The patch will change the behavior if map.contains(key) and fs != map.get(key) . I'm thinking the method still needs double lookups as follows: synchronized void remove(Key key, FileSystem fs) { assert fs != null ; if (map.get(key) == fs) { map.remove(key); toAutoClose.remove(key); } } By the way, the method has two callers. One caller FileSystem.Cache#closeAll //remove from cache remove(key, fs); can be changed as //remove from cache map.remove(key); toAutoClose.remove(key); since map.contains(key) and fs == map.get(key) are guaranteed there. That way we can remove the key from the map by single lookup.
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Thanks a lot for review..Updated the patch based on your review comments..Kindly Review..

        Show
        brahmareddy Brahma Reddy Battula added a comment - Thanks a lot for review..Updated the patch based on your review comments..Kindly Review..
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Minor nit: Would you fix the indent also? I'm +1 if that is addressed.

        +      if ( fs == map.get(key)) {
                 map.remove(key);
                 toAutoClose.remove(key);
                 } // remove 2 whitespaces before "}" 
        

        Gera Shegalov do you have any comments?

        Show
        ajisakaa Akira Ajisaka added a comment - Minor nit: Would you fix the indent also? I'm +1 if that is addressed. + if ( fs == map.get(key)) { map.remove(key); toAutoClose.remove(key); } // remove 2 whitespaces before "}" Gera Shegalov do you have any comments?
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12703600/HADOOP-11659-003.patch
        against trunk revision 7711049.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5906//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5906//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12703600/HADOOP-11659-003.patch against trunk revision 7711049. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5906//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5906//console This message is automatically generated.
        Hide
        jira.shegalov Gera Shegalov added a comment -

        fwiw we can do instead of 2 lookups via get and remove, just remove, if it was wrong (unlikely) just put it back:

              FileSystem cachedFs = map.remove(key);
              if (fs == cachedFs) {
                toAutoClose.remove(key);
              } else if (cachedFs != null) {
                map.put(key, cachedFs);
              }
        
        Show
        jira.shegalov Gera Shegalov added a comment - fwiw we can do instead of 2 lookups via get and remove, just remove, if it was wrong (unlikely) just put it back: FileSystem cachedFs = map.remove(key); if (fs == cachedFs) { toAutoClose.remove(key); } else if (cachedFs != null ) { map.put(key, cachedFs); }
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Thanks for review,Updated the patch ..Kindly review..

        Show
        brahmareddy Brahma Reddy Battula added a comment - Thanks for review,Updated the patch ..Kindly review..
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12704128/HADDOOP-11659-004.patch
        against trunk revision ff83ae7.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5923//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5923//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704128/HADDOOP-11659-004.patch against trunk revision ff83ae7. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5923//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5923//console This message is automatically generated.
        Hide
        jira.shegalov Gera Shegalov added a comment -

        LGTM.
        What do you think, Akira Ajisaka? This is a minor JIRA, I would say we don't need it in 2.7.

        Show
        jira.shegalov Gera Shegalov added a comment - LGTM. What do you think, Akira Ajisaka ? This is a minor JIRA, I would say we don't need it in 2.7.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Thanks Gera Shegalov for the comment and thanks Brahma Reddy Battula for the update. +1 for v4 patch.

        Show
        ajisakaa Akira Ajisaka added a comment - Thanks Gera Shegalov for the comment and thanks Brahma Reddy Battula for the update. +1 for v4 patch.
        Hide
        ajisakaa Akira Ajisaka added a comment -

        Committed this to trunk and branch-2. Thank you Brahma Reddy Battula and Gera Shegalov!

        Show
        ajisakaa Akira Ajisaka added a comment - Committed this to trunk and branch-2. Thank you Brahma Reddy Battula and Gera Shegalov !
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7356 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7356/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7356 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7356/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Hide
        brahmareddy Brahma Reddy Battula added a comment -

        Thanks a lot Gera Shegalov and Akira Ajisaka !!!

        Show
        brahmareddy Brahma Reddy Battula added a comment - Thanks a lot Gera Shegalov and Akira Ajisaka !!!
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #136 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/136/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #136 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/136/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #870 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/870/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #870 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/870/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2068 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2068/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2068 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2068/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #127 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/127/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #127 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/127/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2086 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2086/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2086 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2086/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #136 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/136/)
        HADOOP-11659. o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #136 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/136/ ) HADOOP-11659 . o.a.h.FileSystem.Cache#remove should use a single hash map lookup. Contributed by Brahma Reddy Battula. (aajisaka: rev 34117325b29f0f1bdbe21343e7fd07e9ad0af907) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

          People

          • Assignee:
            brahmareddy Brahma Reddy Battula
            Reporter:
            jira.shegalov Gera Shegalov
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development