Hadoop Common
  1. Hadoop Common
  2. HADOOP-6677

InterfaceAudience.LimitedPrivate should take a string not an enum

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Trying to keep the list of all possible components sharing limited private interfaces up to date is painful. As other subprojects beyond HDFS and MR use this interface it will only get worse (see PIG-1311). If it is converted to a string other subprojects can use it without requiring patches to common.

      1. HADOOP-6677.patch
        9 kB
        Tom White
      2. HADOOP-6677.patch
        10 kB
        Tom White

        Activity

        Hide
        Tom White added a comment -

        +1 However, I wonder if we could go further and not list projects at all. It seems odd to mandate a pre-determined audience for a particular API. To me, it seems better to say "this is a developer-level interface" (stability is independent), which may be used by different projects. A Private API, by contrast, is one that may not be used outside the project.

        With this simplification, the LimitedPrivate annotation would be interpreted as being intended for developers rather than users of Hadoop. This would allow any project to use the API, including subprojects, contrib projects, or projects that are not a part of Hadoop.

        For example, by marking the MapReduce interfaces that schedulers need limited-private the scheduler implementations could be hosted anywhere - as contrib modules as they are today, or as an independent project - it shouldn't matter. Futhermore, by making the Java visibility public, the scheduler implementations would not have to be in the same package as the interfaces, as they are today. Users would not see these interfaces, since they are not in public javadoc.

        Show
        Tom White added a comment - +1 However, I wonder if we could go further and not list projects at all. It seems odd to mandate a pre-determined audience for a particular API. To me, it seems better to say "this is a developer-level interface" (stability is independent), which may be used by different projects. A Private API, by contrast, is one that may not be used outside the project. With this simplification, the LimitedPrivate annotation would be interpreted as being intended for developers rather than users of Hadoop. This would allow any project to use the API, including subprojects, contrib projects, or projects that are not a part of Hadoop. For example, by marking the MapReduce interfaces that schedulers need limited-private the scheduler implementations could be hosted anywhere - as contrib modules as they are today, or as an independent project - it shouldn't matter. Futhermore, by making the Java visibility public, the scheduler implementations would not have to be in the same package as the interfaces, as they are today. Users would not see these interfaces, since they are not in public javadoc.
        Hide
        Alan Gates added a comment -

        The thing I like about having a component name associated with the limited private interface is it tells developers who depends on that interface. So if they want to change it they know who to talk to. But I don't see it as critical. If others feel it's better removed, I'm ok with that.

        Show
        Alan Gates added a comment - The thing I like about having a component name associated with the limited private interface is it tells developers who depends on that interface. So if they want to change it they know who to talk to. But I don't see it as critical. If others feel it's better removed, I'm ok with that.
        Hide
        Tom White added a comment -

        Perhaps it could be optional - if no components are listed, then that means that any component may use it.

        Show
        Tom White added a comment - Perhaps it could be optional - if no components are listed, then that means that any component may use it.
        Hide
        Tom White added a comment -

        Here's a patch to change the enum to a String.

        Show
        Tom White added a comment - Here's a patch to change the enum to a String.
        Hide
        Hadoop QA added a comment -

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

        +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 passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/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/12440988/HADOOP-6677.patch against trunk revision 931226. +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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/444/console This message is automatically generated.
        Hide
        Jakob Homan added a comment -

        This would be a pretty big change to the classification scheme that was argued out quite exhaustively in HADOOP-5073; there should be time for people to re-hash if they wish before anything is committed. Nicholas had made the point there that having an enum avoids problems with typos and such, but this is a minor benefit. If we're removing the limitation of who should use LimitedPrivate, and who can depend on it, then it's not really limited private anymore?

        Show
        Jakob Homan added a comment - This would be a pretty big change to the classification scheme that was argued out quite exhaustively in HADOOP-5073 ; there should be time for people to re-hash if they wish before anything is committed. Nicholas had made the point there that having an enum avoids problems with typos and such, but this is a minor benefit. If we're removing the limitation of who should use LimitedPrivate, and who can depend on it, then it's not really limited private anymore?
        Hide
        Tom White added a comment -

        > This would be a pretty big change to the classification scheme that was argued out quite exhaustively in HADOOP-5073; there should be time for people to re-hash if they wish before anything is committed.

        I agree that people should have a chance to discuss this change. As the annotations begin to get more traction, it seems reasonable to be able to change them to make them better fit how they are actually being used.

        > If we're removing the limitation of who should use LimitedPrivate, and who can depend on it, then it's not really limited private anymore?

        The code that is annotated LimitedPrivate gets to declare who may use it, so in that sense it is still limited private, no?

        Show
        Tom White added a comment - > This would be a pretty big change to the classification scheme that was argued out quite exhaustively in HADOOP-5073 ; there should be time for people to re-hash if they wish before anything is committed. I agree that people should have a chance to discuss this change. As the annotations begin to get more traction, it seems reasonable to be able to change them to make them better fit how they are actually being used. > If we're removing the limitation of who should use LimitedPrivate, and who can depend on it, then it's not really limited private anymore? The code that is annotated LimitedPrivate gets to declare who may use it, so in that sense it is still limited private, no?
        Hide
        Jakob Homan added a comment -

        I'm OK with the switch to strings; it'll make the annotation more useful to other projects, as Alan said, and it's not like we have any real control over someone else using the APIs. My concern is more about the possibility of making it optional; For LimitedPrivate, in Hadoop at least, we should always be clear about who we're including. Making the list of targets optional would make LimitedPrivate just a confusing version of Private.

        Show
        Jakob Homan added a comment - I'm OK with the switch to strings; it'll make the annotation more useful to other projects, as Alan said, and it's not like we have any real control over someone else using the APIs. My concern is more about the possibility of making it optional; For LimitedPrivate, in Hadoop at least, we should always be clear about who we're including. Making the list of targets optional would make LimitedPrivate just a confusing version of Private.
        Hide
        Jakob Homan added a comment -

        So basically, +1 on this patch. Any changes to the semantics of LiimitedPrivate should be hashed out in another JIRA.

        Show
        Jakob Homan added a comment - So basically, +1 on this patch. Any changes to the semantics of LiimitedPrivate should be hashed out in another JIRA.
        Hide
        Tom White added a comment -

        I agree we can discuss a change in semantics elsewhere. I'll commit this tomorrow (Monday) as it will be good to get this change in before they appear in a release, and we start applying the annotations more widely in HADOOP-6668 and MAPREDUCE-1623.

        Show
        Tom White added a comment - I agree we can discuss a change in semantics elsewhere. I'll commit this tomorrow (Monday) as it will be good to get this change in before they appear in a release, and we start applying the annotations more widely in HADOOP-6668 and MAPREDUCE-1623 .
        Hide
        Tom White added a comment -

        Updated patch for trunk.

        Show
        Tom White added a comment - Updated patch for trunk.
        Hide
        Hadoop QA added a comment -

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

        +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 passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/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/12442921/HADOOP-6677.patch against trunk revision 938322. +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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/482/console This message is automatically generated.
        Hide
        Tom White added a comment -

        I've just committed this.

        Show
        Tom White added a comment - I've just committed this.

          People

          • Assignee:
            Tom White
            Reporter:
            Alan Gates
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development