Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4.0
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      In order to intelligently schedule incoming calls, we need to know what identity it falls under.

      We do this by defining the Schedulable interface, which has one method, getIdentity(IdentityType idType)

      The scheduler can then query a Schedulable object for its identity, depending on what idType is.

      For example:
      Call 1: Made by user=Alice, group=admins
      Call 2: Made by user=Bob, group=admins
      Call 3: Made by user=Carlos, group=users
      Call 4: Made by user=Alice, group=admins

      Depending on what the identity is, we would treat these requests differently. If we query on Username, we can bucket these 4 requests into 3 sets for Alice, Bob, and Carlos. If we query on Groupname, we can bucket these 4 requests into 2 sets for admins and users.

      In this initial version, idType can be username or primary group. In future versions, it could be jobID, request class (read or write), or some explicit QoS field. These are user-defined, and will be reloaded on callqueue refresh.

      1. HADOOP-10280.patch
        4 kB
        Chris Li
      2. HADOOP-10280.patch
        7 kB
        Chris Li
      3. HADOOP-10280.patch
        11 kB
        Chris Li
      4. HADOOP-10280.patch
        10 kB
        Chris Li

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1738 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1738/)
        HADOOP-10280. Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java
          HADOOP-10280. Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532)
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1738 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1738/ ) HADOOP-10280 . Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java HADOOP-10280 . Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Hdfs-trunk #1713 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1713/)
        HADOOP-10280. Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java
          HADOOP-10280. Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532)
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1713 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1713/ ) HADOOP-10280 . Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java HADOOP-10280 . Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #521 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/521/)
        HADOOP-10280. Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java
          HADOOP-10280. Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532)
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #521 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/521/ ) HADOOP-10280 . Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java HADOOP-10280 . Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Hide
        chrilisf Chris Li added a comment -

        Thanks Arapit! This completes the core changes required for RPC-level QoS.

        Show
        chrilisf Chris Li added a comment - Thanks Arapit! This completes the core changes required for RPC-level QoS.
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-trunk-Commit #5401 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5401/)
        HADOOP-10280. Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java
          HADOOP-10280. Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532)
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5401 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5401/ ) HADOOP-10280 . Add files missed in previous checkin. (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581533 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/IdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Schedulable.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/UserIdentityProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIdentityProviders.java HADOOP-10280 . Make Schedulables return a configurable identity of user or group. (Contributed by Chris Li) (arp: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1581532 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeys.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        I committed this to trunk, branch-2 and branch-2.4.

        Thanks for the contribution Chris Li!

        Show
        arpitagarwal Arpit Agarwal added a comment - I committed this to trunk, branch-2 and branch-2.4. Thanks for the contribution Chris Li!
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Thanks Chris.

        I intend to commit this later today if there are no objections.

        Show
        arpitagarwal Arpit Agarwal added a comment - Thanks Chris. I intend to commit this later today if there are no objections.
        Hide
        chrilisf Chris Li added a comment -

        Hi Arapit, I think the closest example of how to use the IdentityProvider and Schedulable is in the test cases.

        As far as why we need this capability, qos requires us to extract some identity, usually username. This patch allows that as well as more general classification of Calls.

        Thanks,

        Chris

        Show
        chrilisf Chris Li added a comment - Hi Arapit, I think the closest example of how to use the IdentityProvider and Schedulable is in the test cases. As far as why we need this capability, qos requires us to extract some identity, usually username. This patch allows that as well as more general classification of Calls. Thanks, Chris
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Hi Chris Li, the change looks fine to me. It is kind of hard for me to visualize how Schedulable#getUserGroupInformation and UserIdentityProvider are going to be used without a concrete example but that's not necessarily an issue with the patch.

        I'd be +1 on committing it it if Daryn has no objection.

        Show
        arpitagarwal Arpit Agarwal added a comment - Hi Chris Li , the change looks fine to me. It is kind of hard for me to visualize how Schedulable#getUserGroupInformation and UserIdentityProvider are going to be used without a concrete example but that's not necessarily an issue with the patch. I'd be +1 on committing it it if Daryn has no objection.
        Hide
        arpitagarwal Arpit Agarwal added a comment -

        Daryn Sharp do you have any comments on the latest patch? Thanks, Arpit.

        Show
        arpitagarwal Arpit Agarwal added a comment - Daryn Sharp do you have any comments on the latest patch? Thanks, Arpit.
        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/12635695/HADOOP-10280.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 1 new or modified test files.

        +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 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 unit tests in hadoop-common-project/hadoop-common.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3685//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3685//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/12635695/HADOOP-10280.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +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 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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3685//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3685//console This message is automatically generated.
        Hide
        chrilisf Chris Li added a comment -

        New patch removes all other getters except getUserGroupInformation

        Show
        chrilisf Chris Li added a comment - New patch removes all other getters except getUserGroupInformation
        Hide
        chrilisf Chris Li added a comment -

        I think this is the case with the identity providers, right? Any IdentityProvider which schedules based on knowledge that's not available at the rpc layer will not be in the ipc package, but the application.

        The Schedulable interface will expose internal members, but it's up to the IdentityProvider to make sense of it. In this way, we maintain separation of concerns.

        I'll attach a new patch shortly which only exposes the UGI, since committing to an interface for the other stuff might be too difficult.

        Show
        chrilisf Chris Li added a comment - I think this is the case with the identity providers, right? Any IdentityProvider which schedules based on knowledge that's not available at the rpc layer will not be in the ipc package, but the application. The Schedulable interface will expose internal members, but it's up to the IdentityProvider to make sense of it. In this way, we maintain separation of concerns. I'll attach a new patch shortly which only exposes the UGI, since committing to an interface for the other stuff might be too difficult.
        Hide
        daryn Daryn Sharp added a comment -

        Haven't quite figured how to do that cleanly yet

        That's because you can't at this layer w/o terribly violating abstractions. This rpc layer is responsible for accepting connections, reading rpc messages, creating an opaque call object, and queueing it for the handlers. The handlers use a rpc invoker & engine to decode the call and invoke a method in the app's rpc server impl. Nothing is known about the opaque call while it's in or taken out of the callq. If you want to schedule based on something other than source address, remote user, or rpc header fields, then the scheduling needs to be part of the app's rpc server impl.

        I've wanted to do a type of scheduling of method invocation in the NN and concluded that I should reduce the number of handlers (possibly to 1), the rpc server impl does custom scheduling/queuing, and the impl maintains it's own set of handlers for consuming from its custom queue.

        All said, I'm comfortable with adding call.getRemoteUser() which returns the connection's remote user.

        Show
        daryn Daryn Sharp added a comment - Haven't quite figured how to do that cleanly yet That's because you can't at this layer w/o terribly violating abstractions. This rpc layer is responsible for accepting connections, reading rpc messages, creating an opaque call object, and queueing it for the handlers. The handlers use a rpc invoker & engine to decode the call and invoke a method in the app's rpc server impl. Nothing is known about the opaque call while it's in or taken out of the callq. If you want to schedule based on something other than source address, remote user, or rpc header fields, then the scheduling needs to be part of the app's rpc server impl. I've wanted to do a type of scheduling of method invocation in the NN and concluded that I should reduce the number of handlers (possibly to 1), the rpc server impl does custom scheduling/queuing, and the impl maintains it's own set of handlers for consuming from its custom queue. All said, I'm comfortable with adding call.getRemoteUser() which returns the connection's remote user.
        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/12635505/HADOOP-10280.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 1 new or modified test files.

        +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 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 unit tests in hadoop-common-project/hadoop-common.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3681//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3681//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/12635505/HADOOP-10280.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +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 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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3681//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3681//console This message is automatically generated.
        Hide
        chrilisf Chris Li added a comment -

        Attached new version of patch which uses class based IdentityProviders to determine identity. This will be more flexible going forward.

        As a requirement of this, internal members of Call are made available through the Schedulable interface.

        One such IdentityProvider, the UserIdentityProvider, is included, which will serve as the default.

        Show
        chrilisf Chris Li added a comment - Attached new version of patch which uses class based IdentityProviders to determine identity. This will be more flexible going forward. As a requirement of this, internal members of Call are made available through the Schedulable interface. One such IdentityProvider, the UserIdentityProvider, is included, which will serve as the default.
        Hide
        chrilisf Chris Li added a comment -

        Haven't quite figured how to do that cleanly yet. If the information is available in the Call class somewhere, we could make it public and then specify an adapter class to handle forming an identity string.

        We'd be making most of the members of Call public, but this approach would be most flexible. Let me know if that sounds reasonable.

        Show
        chrilisf Chris Li added a comment - Haven't quite figured how to do that cleanly yet. If the information is available in the Call class somewhere, we could make it public and then specify an adapter class to handle forming an identity string. We'd be making most of the members of Call public, but this approach would be most flexible. Let me know if that sounds reasonable.
        Hide
        daryn Daryn Sharp added a comment -

        How would you determine a jobid or read vs write call? The rpc component is common so I'm unclear how the jobid can be determined w/o making rpc NN-specific? The rpc layer also has no idea of the app specific proxy's semantics to determine read vs write.

        Show
        daryn Daryn Sharp added a comment - How would you determine a jobid or read vs write call? The rpc component is common so I'm unclear how the jobid can be determined w/o making rpc NN-specific? The rpc layer also has no idea of the app specific proxy's semantics to determine read vs write.
        Hide
        chrilisf Chris Li added a comment -

        Daryn Sharp thanks for taking a look. I've done it like this because sometimes you may want to schedule on something that isn't the user, such as the associated MR JobID, or the call type (treating writes differently than reads).

        I could have Schedulable expose a getUserGroupInformation today, and eventually a getJobID, getCallType, if that sounds more straightforward.

        Show
        chrilisf Chris Li added a comment - Daryn Sharp thanks for taking a look. I've done it like this because sometimes you may want to schedule on something that isn't the user, such as the associated MR JobID, or the call type (treating writes differently than reads). I could have Schedulable expose a getUserGroupInformation today, and eventually a getJobID, getCallType, if that sounds more straightforward.
        Hide
        daryn Daryn Sharp added a comment -

        Why not use the ugi from the connection object? Using an enum to extract ugi related fields (possibly returning a "?") seems clunky.

        Show
        daryn Daryn Sharp added a comment - Why not use the ugi from the connection object? Using an enum to extract ugi related fields (possibly returning a "?") seems clunky.
        Hide
        chrilisf Chris Li added a comment -

        Hi, can anybody weigh in?

        Show
        chrilisf Chris Li added a comment - Hi, can anybody weigh in?
        Hide
        chrilisf Chris Li added a comment -

        Hey Daryn Sharp just wanted to see if you found time to check it out. Thanks

        Show
        chrilisf Chris Li added a comment - Hey Daryn Sharp just wanted to see if you found time to check it out. Thanks
        Hide
        daryn Daryn Sharp added a comment -

        I'm getting my head above water. I'll try to review soon.

        Show
        daryn Daryn Sharp added a comment - I'm getting my head above water. I'll try to review soon.
        Hide
        chrilisf Chris Li added a comment -

        Makes sense, but in order to do the above, we'd have to

        1. Refactor Call into its own class outside of Server
        2. Add getters to allow access to Call's members, probably all of them in order to be most flexible

        It is most flexible, but I wonder if it's overkill.

        Another point : Shouldn't the code that uses Schedulable be part of this patch as well ?

        I think it's best to keep that in a later patch, for focus. The scheduler has its own quirks and things to be debated I'm sure

        Show
        chrilisf Chris Li added a comment - Makes sense, but in order to do the above, we'd have to 1. Refactor Call into its own class outside of Server 2. Add getters to allow access to Call's members, probably all of them in order to be most flexible It is most flexible, but I wonder if it's overkill. Another point : Shouldn't the code that uses Schedulable be part of this patch as well ? I think it's best to keep that in a later patch, for focus. The scheduler has its own quirks and things to be debated I'm sure
        Hide
        benoyantony Benoy Antony added a comment -

        I was confused as well. I was not sure on why Call should implement Schedulable . Now I understand why Call should implement Schedulable.

        The above code gives an opportunity to customize the identity. That was my goal. But it adds complexity.

        Another point : Shouldn't the code that uses Schedulable be part of this patch as well ?

        Show
        benoyantony Benoy Antony added a comment - I was confused as well. I was not sure on why Call should implement Schedulable . Now I understand why Call should implement Schedulable . The above code gives an opportunity to customize the identity. That was my goal. But it adds complexity. Another point : Shouldn't the code that uses Schedulable be part of this patch as well ?
        Hide
        chrilisf Chris Li added a comment -

        Still not exactly sure. Do you mean that call should be composed of an object that provides the identity? Something like the adapter pattern:

        Server.java
        class Call implements Schedulable {
          private SchedulableIdentityProvider myProvider;
        
          public Call(options) {
            this.myProvider = conf.instantiateClass(“ipc.schedulable.identity-provider”)
          }
        
          public getIdentity() {
            return myProvider.identityFor(this);
          }
        }
        
        SchedulableIdentityProvider.java
        interface SchedulableIdentityProvider {
          public identityFor(Call c);
        }
        
        UserSchedulableIdentityProvider.java
        class UserSchedulableIdentityProvider implements SchedulableIdentityProvider {
          @Override
          public identityFor(Call c) {
            return c.getUserName();
          }
        }
        
        Show
        chrilisf Chris Li added a comment - Still not exactly sure. Do you mean that call should be composed of an object that provides the identity? Something like the adapter pattern: Server.java class Call implements Schedulable { private SchedulableIdentityProvider myProvider; public Call(options) { this .myProvider = conf.instantiateClass(“ipc.schedulable.identity-provider”) } public getIdentity() { return myProvider.identityFor( this ); } } SchedulableIdentityProvider.java interface SchedulableIdentityProvider { public identityFor(Call c); } UserSchedulableIdentityProvider.java class UserSchedulableIdentityProvider implements SchedulableIdentityProvider { @Override public identityFor(Call c) { return c.getUserName(); } }
        Hide
        benoyantony Benoy Antony added a comment -

        Schedulable needs to be an interface. But I am not sure about Call implementing Schedulable . I think , its better to have to Call using Schedulable interface. The implementation can be provided via config.

        Show
        benoyantony Benoy Antony added a comment - Schedulable needs to be an interface. But I am not sure about Call implementing Schedulable . I think , its better to have to Call using Schedulable interface. The implementation can be provided via config.
        Hide
        chrilisf Chris Li added a comment -

        Not sure if I follow, do you mean to say that we should not use Schedulable as an interface, but add the methods directly to Call?

        Show
        chrilisf Chris Li added a comment - Not sure if I follow, do you mean to say that we should not use Schedulable as an interface, but add the methods directly to Call?
        Hide
        benoyantony Benoy Antony added a comment -

        Chris Li Does it makes sense to add ability to plugin a different implementation of Schedulable ? If so , Call implementing Schedulable may not be a good idea. Call could use the configured Schedulable implementation.

        Show
        benoyantony Benoy Antony added a comment - Chris Li Does it makes sense to add ability to plugin a different implementation of Schedulable ? If so , Call implementing Schedulable may not be a good idea. Call could use the configured Schedulable implementation.
        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/12631592/HADOOP-10280.patch
        against trunk revision .

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

        +1 tests included. The patch appears to include 1 new or modified test files.

        +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 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 unit tests in hadoop-common-project/hadoop-common.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3617//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3617//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/12631592/HADOOP-10280.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +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 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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3617//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3617//console This message is automatically generated.
        Hide
        chrilisf Chris Li added a comment -

        Add a test

        Show
        chrilisf Chris Li added a comment - Add a test
        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/12631410/HADOOP-10280.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 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 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 unit tests in hadoop-common-project/hadoop-common.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3612//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3612//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/12631410/HADOOP-10280.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 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 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 unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3612//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3612//console This message is automatically generated.
        Hide
        chrilisf Chris Li added a comment -

        Resubmit to get QA to run

        Show
        chrilisf Chris Li added a comment - Resubmit to get QA to run
        Hide
        chrilisf Chris Li added a comment -

        This version has javadocs and fixes some whitespace

        Show
        chrilisf Chris Li added a comment - This version has javadocs and fixes some whitespace
        Hide
        chrilisf Chris Li added a comment -

        Allow Schedulables to be queried for identity, which can be configured as follows:

        ipc.8020.call.identity

        Show
        chrilisf Chris Li added a comment - Allow Schedulables to be queried for identity, which can be configured as follows: ipc.8020.call.identity

          People

          • Assignee:
            chrilisf Chris Li
            Reporter:
            chrilisf Chris Li
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development