Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-775

FSDataset calls getCapacity() twice -bug?

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: datanode
    • Labels:
      None

      Description

      I'm not sure this is a bug or "as intended", but I thought I'd mention it.

      FSDataset.getCapacity() calls DF.getCapacity() twice, when evaluating its capacity. Although there is caching to stop the shell being exec'd twice in a row, there is a risk that the first call doesn't run the shell, and the second does -so the value changes during the method.

      If that is not intended, it is better to cache the first value for the whole method

      1. HDFS-775-2.patch
        0.9 kB
        steve_l
      2. HDFS-775-1.patch
        0.9 kB
        steve_l

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/)

        Show
        Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #94 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/94/ )
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #182 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/182/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #182 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/182/ )
        Hide
        Hudson added a comment -

        Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #159 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/159/)
        FSDataset calls getCapacity() twice

        Show
        Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #159 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/159/ ) FSDataset calls getCapacity() twice
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Hi Steve, forgot to update CHANGE.txt?

        Show
        Tsz Wo Nicholas Sze added a comment - Hi Steve, forgot to update CHANGE.txt?
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #156 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/156/)
        FSDataset calls getCapacity() twice

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #156 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/156/ ) FSDataset calls getCapacity() twice
        Hide
        steve_l added a comment -

        committed.

        Show
        steve_l added a comment - committed.
        Hide
        Todd Lipcon added a comment -

        +1. Patch looks good to me.

        Show
        Todd Lipcon added a comment - +1. Patch looks good to me.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12427479/HDFS-775-2.patch
        against trunk revision 888538.

        +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 javadoc. The javadoc tool did not generate any warning messages.

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

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

        +1 core tests. The patch passed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12427479/HDFS-775-2.patch against trunk revision 888538. +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 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/139/console This message is automatically generated.
        Hide
        steve_l added a comment -

        No tests; this is a race condition being eliminated here. Proofs of correctness only are sufficient, and I don't do those.

        Show
        steve_l added a comment - No tests; this is a race condition being eliminated here. Proofs of correctness only are sufficient, and I don't do those.
        Hide
        steve_l added a comment -

        patch with corrected path

        Show
        steve_l added a comment - patch with corrected path
        Hide
        steve_l added a comment -

        This is a patch for this; I am using this defect to understand how to work with git/github for patches, so apologies if it does not apply properly - the path looks odd and may need a -p1 to take

        Show
        steve_l added a comment - This is a patch for this; I am using this defect to understand how to work with git/github for patches, so apologies if it does not apply properly - the path looks odd and may need a -p1 to take
        Hide
        steve_l added a comment -

        minor defect; its transient and nothing will actually break.

        Show
        steve_l added a comment - minor defect; its transient and nothing will actually break.
        Hide
        steve_l added a comment -

        Here's the code

            long getCapacity() throws IOException {
              if (reserved > usage.getCapacity()) {        //FIRST CALL
                return 0;
              }
        
              return usage.getCapacity()-reserved;         //SECOND CALL
            }
        

        It looks like the method intends to return capacity as a number >=0, but if the second invocation triggers a shell exec the capacity could decrease and the return value could then be negative, which could have implications elsewhere.

        Looking at the usages, FSVolumeSet can get confused by this, as it adds the capacities of all volumes together, no checks for being below zero.

            long getCapacity() throws IOException {
              long capacity = 0L;
              for (int idx = 0; idx < volumes.length; idx++) {
                capacity += volumes[idx].getCapacity();
              }
              return capacity;
            }
        

        A negative capacity from one volume would make the entire datanode capacity appear smaller than it is.

        Show
        steve_l added a comment - Here's the code long getCapacity() throws IOException { if (reserved > usage.getCapacity()) { //FIRST CALL return 0; } return usage.getCapacity()-reserved; //SECOND CALL } It looks like the method intends to return capacity as a number >=0, but if the second invocation triggers a shell exec the capacity could decrease and the return value could then be negative, which could have implications elsewhere. Looking at the usages, FSVolumeSet can get confused by this, as it adds the capacities of all volumes together, no checks for being below zero. long getCapacity() throws IOException { long capacity = 0L; for ( int idx = 0; idx < volumes.length; idx++) { capacity += volumes[idx].getCapacity(); } return capacity; } A negative capacity from one volume would make the entire datanode capacity appear smaller than it is.

          People

          • Assignee:
            Steve Loughran
            Reporter:
            Steve Loughran
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development