Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Release Note:
      labeled the method @Deprecated as suggested by dhruba

      Description

      DFSClient#getBlockSize is unused. Since it's a public class internal to HDFS we just remove it? If not then we should add a unit test.

      1. HDFS-929-take1.txt
        0.5 kB
        Jim Plush
      2. HDFS-929.patch
        4 kB
        Harsh J

        Activity

        Hide
        dhruba borthakur added a comment -

        Maybe you can mark it as deprecated?

        Show
        dhruba borthakur added a comment - Maybe you can mark it as deprecated?
        Hide
        Jim Plush added a comment -

        Added the @Deprecated tag to the method as suggested.

        Show
        Jim Plush added a comment - Added the @Deprecated tag to the method as suggested.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12483825/HDFS-929-take1.txt
        against trunk revision 1139473.

        +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 (version 1.3.9) 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 passed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/841//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/841//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/841//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/12483825/HDFS-929-take1.txt against trunk revision 1139473. +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 (version 1.3.9) 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 passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/841//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/841//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/841//console This message is automatically generated.
        Hide
        dhruba borthakur added a comment -

        any motivation for doing this? Since this is a public api, other software might be using it maybe? is there any downside of keeping this API as it is?

        Show
        dhruba borthakur added a comment - any motivation for doing this? Since this is a public api, other software might be using it maybe? is there any downside of keeping this API as it is?
        Hide
        Jim Plush added a comment -

        apologies, I read your comment "Maybe you can mark it as deprecated?" as you wanted it marked deprecated.

        Show
        Jim Plush added a comment - apologies, I read your comment "Maybe you can mark it as deprecated?" as you wanted it marked deprecated.
        Hide
        Harsh J added a comment -

        Thanks for the careful eye Jim!

        This is provided by the ClientProtocol. If it is being deprecated, it should be deprecated all the way, and removed at a subsequent release. This'd incur an unwanted change in compatibility and will make HDFS require major version upgrade. So far, I only see server-side mechanisms use this via the INodeFile#getPreferredBlockSize(…) method that eventually gets called, and this is called pretty directly.

        I do not know why its published into the client (seems to be available since very long ago) but there's no harm in keeping it as DFSClient was never meant for user consumption. Perhaps a feature that may get added later may use it?

        If you agree with this assessment, lets resolve this one.

        Show
        Harsh J added a comment - Thanks for the careful eye Jim! This is provided by the ClientProtocol. If it is being deprecated, it should be deprecated all the way, and removed at a subsequent release. This'd incur an unwanted change in compatibility and will make HDFS require major version upgrade. So far, I only see server-side mechanisms use this via the INodeFile#getPreferredBlockSize(…) method that eventually gets called, and this is called pretty directly. I do not know why its published into the client (seems to be available since very long ago) but there's no harm in keeping it as DFSClient was never meant for user consumption. Perhaps a feature that may get added later may use it? If you agree with this assessment, lets resolve this one.
        Hide
        Eli Collins added a comment -

        Let's mark as deprecated and add a unit test.

        Show
        Eli Collins added a comment - Let's mark as deprecated and add a unit test.
        Hide
        Harsh J added a comment -

        Patch that deprecates the ClientProtocol calls.

        For tests, INodeFile already has a test for its use internally, should we add a test with ClientProtocol use in mind?

        Show
        Harsh J added a comment - Patch that deprecates the ClientProtocol calls. For tests, INodeFile already has a test for its use internally, should we add a test with ClientProtocol use in mind?
        Hide
        Hadoop QA added a comment -

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

        +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 appears to have generated 20 warning messages.

        -1 javac. The applied patch generated 41 javac compiler warnings (more than the trunk's current 39 warnings).

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

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        -1 release audit. The applied patch generated 1 release audit warnings (more than the trunk's current 0 warnings).

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

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

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//artifact/trunk/hadoop-hdfs-project/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//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/12509272/HDFS-929.patch against trunk revision . +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 appears to have generated 20 warning messages. -1 javac. The applied patch generated 41 javac compiler warnings (more than the trunk's current 39 warnings). +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. -1 release audit. The applied patch generated 1 release audit warnings (more than the trunk's current 0 warnings). +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//artifact/trunk/hadoop-hdfs-project/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/1748//console This message is automatically generated.
        Hide
        Allen Wittenauer added a comment -

        Do we want to actually deprecate this now or just let it live on forever?

        Show
        Allen Wittenauer added a comment - Do we want to actually deprecate this now or just let it live on forever?

          People

          • Assignee:
            Jim Plush
            Reporter:
            Eli Collins
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:

              Development