Hadoop Common
  1. Hadoop Common
  2. HADOOP-5419

Provide a way for users to find out what operations they can do on which M/R queues

    Details

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

      Description

      This issue is to provide an improvement on the existing M/R framework to let users know which queues they have access to, and for what operations. One use case for this would that currently there is no easy way to know if the user has access to submit jobs to a queue, until it fails with an access control exception.

      1. hadoop-5419.patch
        16 kB
        rahul k singh
      2. hadoop-5419.patch
        16 kB
        rahul k singh
      3. hadoop-5419.patch
        18 kB
        rahul k singh
      4. hadoop-5419.patch
        18 kB
        rahul k singh
      5. commands_manual.pdf
        42 kB
        rahul k singh
      6. hadoop-5419-1.patch
        18 kB
        rahul k singh
      7. hadoop-5419-1.patch
        16 kB
        rahul k singh
      8. hadoop-5419-2.patch
        9 kB
        rahul k singh
      9. hadoop-5419-2.patch
        17 kB
        rahul k singh
      10. hadoop-5419-3.patch
        20 kB
        rahul k singh
      11. hadoop-5419-4.patch
        20 kB
        rahul k singh
      12. hadoop-5419-5.patch
        19 kB
        rahul k singh
      13. hadoop-5419-6.patch
        19 kB
        rahul k singh
      14. hadoop-5419-6-findbugResolution.patch
        19 kB
        rahul k singh
      15. hadoop-5419-7.patch
        19 kB
        rahul k singh
      16. hadoop-5419-v20.patch
        20 kB
        Robert Chansler
      17. hadoop-5419-v20.2.patch
        14 kB
        Hemanth Yamijala

        Activity

        Hide
        Hemanth Yamijala added a comment -

        If the main requirement is met, we can trivially add support to check if a user has access to a given queue, I think.

        Show
        Hemanth Yamijala added a comment - If the main requirement is met, we can trivially add support to check if a user has access to a given queue, I think.
        Hide
        Nigel Daley added a comment -

        This should be available thru cmd line and web ui.

        Show
        Nigel Daley added a comment - This should be available thru cmd line and web ui.
        Hide
        rahul k singh added a comment - - edited

        Command and O/p

        hadoop queue -showacls

        Queue acls for user :  <username>
        Queue Name : qu1  Operations : acl-submit-job acl-administer-jobs 
        Queue Name : qu3  Operations : acl-submit-job acl-administer-jobs 
        

        New interface method is introduced in JobSubmissionProtocol to fetch the Acls information. This interface provides list of all the queue acls and operations allowed. List only consists of queue for which user has atleast 1 acl.

        New class QueueAclsInfo is introduced to encapsulate Queue name and Queue operation data on the client side.

        QueueAclsInfo.java
        z
        /**
         *  Class to encapsulate Queue ACLs for a particular
         *  user.
         * 
         */
        class QueueAclsInfo implements Writable {
        
          private String queueName;
          private String[] operations;
          /**
           * Default constructor for QueueAclsInfo.
           * 
           */
        

        Added new method to JobSubmissionProtocol

        JobSubmissionProtocol.java
        /**
           * Gets the Queue ACLs for a user
           * @param userName User name
           * @return array of QueueAclsInfo object for a user.
           * @throws IOException
           */
          public QueueAclsInfo[] getQueueAclsInfo(String userName) throws IOException;
        

        Implementation of this method is provided in QueueManager.java

        QueueManager.java
         /**
           * Generates the array of QueueAclsInfo object. The array consists of only those queues
           * for which user has acls
           * 
           * @param username
           * @return QueueAclsInfo[]
           * @throws java.io.IOException
           */
          QueueAclsInfo[] getQueueAclsInfo(String username) throws IOException{
            if(username == null || username.equals(""))
              username = UserGroupInformation.getCurrentUGI().getUserName();
            //List of all QueueAclsInfo objects , this list is returned
            ArrayList<QueueAclsInfo> queueAclsInfolist = new ArrayList<QueueAclsInfo>();
            Iterator<String> iter = queueNames.iterator();
            QueueOperation[] operations = QueueOperation.values();    
            while(iter.hasNext()){      
              String queueName = iter.next();      
              //QueueAclsInfo object for queue queueName, this object is lazily initialized when there is atleast one queue operation
              //supported for the current queue
              QueueAclsInfo queueAclsInfo = null;
              //Initialize operationsAllowed only if atleast 1 operation is supported for user <username>
              //for queue <queueName>
              ArrayList<String> operationsAllowed = null;
              //Check if user has access for particular operations
              for(int i = 0;i < operations.length;i++){
                AccessControlList acl = aclsMap.get(toFullPropertyName(queueName,operations[i].getAclName()));
               if(acl == null){
                 //No acls for this operation
                 continue;
               }else{
                 boolean allowed = acl.allAllowed();
                 if(allowed) { //All users granted access for this operation in queue <queueName>
                   if(operationsAllowed == null) { 
                     operationsAllowed = new ArrayList<String>();
                   }
                   operationsAllowed.add(operations[i].getAclName());           
                 }else { // All users have not been granted access , check if this user <username> is .
                   if(acl.getUsers().contains(username)) {
                     if(operationsAllowed == null)
                       operationsAllowed = new ArrayList<String>();
                     
                     operationsAllowed.add(operations[i].getAclName());
                   }
                 }
               }
              }
              //Check if user username has acls for queue queueName
              //if not no need to create QueueAclsInfo object
              if(operationsAllowed != null) {                
                //There is atleast 1 operation supported for queue <queueName>, hence initialize queueAclsInfo
                queueAclsInfo = new QueueAclsInfo(queueName,operationsAllowed.toArray(new String[operationsAllowed.size()]));
                queueAclsInfolist.add(queueAclsInfo);        
              }
            }
            
            return queueAclsInfolist.toArray(new QueueAclsInfo[queueAclsInfolist.size()]);
          }
        }
        
        Show
        rahul k singh added a comment - - edited Command and O/p hadoop queue -showacls Queue acls for user : <username> Queue Name : qu1 Operations : acl-submit-job acl-administer-jobs Queue Name : qu3 Operations : acl-submit-job acl-administer-jobs New interface method is introduced in JobSubmissionProtocol to fetch the Acls information. This interface provides list of all the queue acls and operations allowed. List only consists of queue for which user has atleast 1 acl. New class QueueAclsInfo is introduced to encapsulate Queue name and Queue operation data on the client side. QueueAclsInfo.java z /** * Class to encapsulate Queue ACLs for a particular * user. * */ class QueueAclsInfo implements Writable { private String queueName; private String [] operations; /** * Default constructor for QueueAclsInfo. * */ Added new method to JobSubmissionProtocol JobSubmissionProtocol.java /** * Gets the Queue ACLs for a user * @param userName User name * @ return array of QueueAclsInfo object for a user. * @ throws IOException */ public QueueAclsInfo[] getQueueAclsInfo( String userName) throws IOException; Implementation of this method is provided in QueueManager.java QueueManager.java /** * Generates the array of QueueAclsInfo object. The array consists of only those queues * for which user has acls * * @param username * @ return QueueAclsInfo[] * @ throws java.io.IOException */ QueueAclsInfo[] getQueueAclsInfo( String username) throws IOException{ if (username == null || username.equals("")) username = UserGroupInformation.getCurrentUGI().getUserName(); //List of all QueueAclsInfo objects , this list is returned ArrayList<QueueAclsInfo> queueAclsInfolist = new ArrayList<QueueAclsInfo>(); Iterator< String > iter = queueNames.iterator(); QueueOperation[] operations = QueueOperation.values(); while (iter.hasNext()){ String queueName = iter.next(); //QueueAclsInfo object for queue queueName, this object is lazily initialized when there is atleast one queue operation //supported for the current queue QueueAclsInfo queueAclsInfo = null ; //Initialize operationsAllowed only if atleast 1 operation is supported for user <username> // for queue <queueName> ArrayList< String > operationsAllowed = null ; //Check if user has access for particular operations for ( int i = 0;i < operations.length;i++){ AccessControlList acl = aclsMap.get(toFullPropertyName(queueName,operations[i].getAclName())); if (acl == null ){ //No acls for this operation continue ; } else { boolean allowed = acl.allAllowed(); if (allowed) { //All users granted access for this operation in queue <queueName> if (operationsAllowed == null ) { operationsAllowed = new ArrayList< String >(); } operationsAllowed.add(operations[i].getAclName()); } else { // All users have not been granted access , check if this user <username> is . if (acl.getUsers().contains(username)) { if (operationsAllowed == null ) operationsAllowed = new ArrayList< String >(); operationsAllowed.add(operations[i].getAclName()); } } } } //Check if user username has acls for queue queueName // if not no need to create QueueAclsInfo object if (operationsAllowed != null ) { //There is atleast 1 operation supported for queue <queueName>, hence initialize queueAclsInfo queueAclsInfo = new QueueAclsInfo(queueName,operationsAllowed.toArray( new String [operationsAllowed.size()])); queueAclsInfolist.add(queueAclsInfo); } } return queueAclsInfolist.toArray( new QueueAclsInfo[queueAclsInfolist.size()]); } }
        Hide
        rahul k singh added a comment -

        uploading the patch

        Show
        rahul k singh added a comment - uploading the patch
        Hide
        rahul k singh added a comment - - edited

        to answer nigel comments:

        This should be available thru cmd line and web ui.

        As of now there is no way of authentication for user on web ui . So we cannot show per user data.
        Moreover showing acls for all the user's ,imho, might not be such a good thing.

        Show
        rahul k singh added a comment - - edited to answer nigel comments: This should be available thru cmd line and web ui. As of now there is no way of authentication for user on web ui . So we cannot show per user data. Moreover showing acls for all the user's ,imho, might not be such a good thing.
        Hide
        rahul k singh added a comment -

        ran the test patch

        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]

        Show
        rahul k singh added a comment - ran the test patch [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] -1 javadoc. The javadoc tool appears to have generated 1 warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
        Hide
        Sreekanth Ramakrishnan added a comment -

        Looked at the patch uploaded. Following are the comments which I had while looking at the code:

        • Bump up the JobSubmissionProtocol version by one.
        • Formatting change, can we change the output format to following:
          Queue acls for user :  <username>
          Queue Name : qu1  Job Operations : submit  administer 
          Queue Name : qu2  Job Operations : submit
          
        • Can't just define a toString() method in QueueAclsInfo to directly present the string in the way it has to be printed to Console?
        • Why has getUGI() in JobClient made Public? Can we figure out other methods by which we can see to that visiblity of the method is not changed to public unless it is a pressing reason.
        • Shouldn't this new method getQueueAclsInfo in QueueManager be thread-safe? Because HADOOP-5396 can refresh queue acls while user is looking thro' ACL maps which QueueManager uses.
        • Minor nit enclose if block in getQueueAclsInfo in QueueManager in line 337.
        Show
        Sreekanth Ramakrishnan added a comment - Looked at the patch uploaded. Following are the comments which I had while looking at the code: Bump up the JobSubmissionProtocol version by one. Formatting change, can we change the output format to following: Queue acls for user : <username> Queue Name : qu1 Job Operations : submit administer Queue Name : qu2 Job Operations : submit Can't just define a toString() method in QueueAclsInfo to directly present the string in the way it has to be printed to Console? Why has getUGI() in JobClient made Public? Can we figure out other methods by which we can see to that visiblity of the method is not changed to public unless it is a pressing reason. Shouldn't this new method getQueueAclsInfo in QueueManager be thread-safe? Because HADOOP-5396 can refresh queue acls while user is looking thro' ACL maps which QueueManager uses. Minor nit enclose if block in getQueueAclsInfo in QueueManager in line 337.
        Hide
        rahul k singh added a comment -

        attaching the patch and handled some of the scenarios mentioned by srikanth.

        Show
        rahul k singh added a comment - attaching the patch and handled some of the scenarios mentioned by srikanth.
        Hide
        Nigel Daley added a comment -

        New command line commands and options should be tested in TestCLI and include valid and invalid inputs. Karthikeyan may be able to provide some help as he worked on HADOOP-5080.

        Show
        Nigel Daley added a comment - New command line commands and options should be tested in TestCLI and include valid and invalid inputs. Karthikeyan may be able to provide some help as he worked on HADOOP-5080 .
        Hide
        rahul k singh added a comment -

        uploaded the latest patch with suggestions after discussion with hemanth.

        In addition mentioning some points about the fix , which weren't very clear from my update above.

        -Added new API in JobSubmissionProtocol getQueueAclsInfo.it takes username and returns QueueAclsInfo class

        -Created new QueueAclsInfo class , it is a Writable class.

        -QueueManager implements the method and synchronized. The method is synchronized because aclsMap in QueueManger might get (incase of automatic acls refresh feature)refreshed.This if happens will lead to issues.

        -In JobClient we are doing

            ugi = UnixUserGroupInformation.login(job, true);
              UnixUserGroupInformation.setCurrentUser(ugi);

        This is done so that we get ugi value in JobQueueClient which displaying the acls info , as we are displaying username. Anythoughts?

        -We had discussed about the web ui part , we thought that any user seeing other acls might lead to some issues. If at all we allow this we need to define special kind of user permissions (for example : administrator) so that those users can view the acls of all the users.

        Show
        rahul k singh added a comment - uploaded the latest patch with suggestions after discussion with hemanth. In addition mentioning some points about the fix , which weren't very clear from my update above. -Added new API in JobSubmissionProtocol getQueueAclsInfo.it takes username and returns QueueAclsInfo class -Created new QueueAclsInfo class , it is a Writable class. -QueueManager implements the method and synchronized. The method is synchronized because aclsMap in QueueManger might get (incase of automatic acls refresh feature)refreshed.This if happens will lead to issues. -In JobClient we are doing ugi = UnixUserGroupInformation.login(job, true ); UnixUserGroupInformation.setCurrentUser(ugi); This is done so that we get ugi value in JobQueueClient which displaying the acls info , as we are displaying username. Anythoughts? -We had discussed about the web ui part , we thought that any user seeing other acls might lead to some issues. If at all we allow this we need to define special kind of user permissions (for example : administrator) so that those users can view the acls of all the users.
        Hide
        rahul k singh added a comment -

        ran test patch.
        output:
        ====
        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]

        Show
        rahul k singh added a comment - ran test patch. output: ==== [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec]
        Hide
        Sreekanth Ramakrishnan added a comment -

        Took a look at the changes, everything is fine, expect few comments:

        • Why are we using a MiniMRCluster for test case? Why cant we directly use QueueManager.getQueueAclsInfo(String) that would help us reduce run time of the test case.
        • Why cant we move the replacing of "acl-" in toString() method of QueueAclsInfo so that in your JobQueueClient.displayQueueAclsInfoForCurrentUser() we can just do following
           for(QueueAclsInfo queueinfo: queueAclsInfo){
                System.out.println(queueinfo);
              }
          
        • Also, we should be adding this to TestCLI, we should do that as a part of a Seperate JIRA, or HADOOP-5080 if it is not commited.
        • Please document the new command in hadoop commands manual

        Minor nit:

        • Keep line width not more than 80 characters in QueueManager.getQueueAclsInfo(String)
        Show
        Sreekanth Ramakrishnan added a comment - Took a look at the changes, everything is fine, expect few comments: Why are we using a MiniMRCluster for test case? Why cant we directly use QueueManager.getQueueAclsInfo(String) that would help us reduce run time of the test case. Why cant we move the replacing of "acl-" in toString() method of QueueAclsInfo so that in your JobQueueClient.displayQueueAclsInfoForCurrentUser() we can just do following for(QueueAclsInfo queueinfo: queueAclsInfo){ System.out.println(queueinfo); } Also, we should be adding this to TestCLI , we should do that as a part of a Seperate JIRA, or HADOOP-5080 if it is not commited. Please document the new command in hadoop commands manual Minor nit: Keep line width not more than 80 characters in QueueManager.getQueueAclsInfo(String)
        Hide
        rahul k singh added a comment -

        uploading the patch.
        Regarding TestCli , would check if this testcase can be added at this point or else would open a seperate jira for that as mentioned in comment earlier.

        Show
        rahul k singh added a comment - uploading the patch. Regarding TestCli , would check if this testcase can be added at this point or else would open a seperate jira for that as mentioned in comment earlier.
        Hide
        rahul k singh added a comment -

        adding the new commands_manual.pdf after changes in xdocs

        Show
        rahul k singh added a comment - adding the new commands_manual.pdf after changes in xdocs
        Hide
        rahul k singh added a comment -

        run test patch result
        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]
        [exec]

        Show
        rahul k singh added a comment - run test patch result [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec]
        Hide
        Karam Singh added a comment -

        After applying the latest patch, ran some : mapred. queue -showacls commands and found one issue and one minor enhancement:
        Issue -:
        In mapred-site.xml for queue acls if we specify user gorups instead of user queue -showacls command gives black list in console.
        E.g
        Have two queue: Q1,Q2
        Q1 acl: SUBMIT_JOB=u1,u2 and ADMIN_JOB=U3
        Q2 acls SUBMIT_JOB= UG1(where UG1 is user group) and ADMIN_JOB=U3

        Now use commad: mapred queue -showacls (with user U5 who is part of UG1 group)
        It shows -:
        [
        Queue acls for user : U5

        ]
        Where U5 can submit job to Q2

        Enhancemnet-:
        Consider same case and use comamnd : mapred queue -showacls (with user U4 who is not part of UG1 group).
        It shows -:
        [
        Queue acls for user : U5

        ]

        It would be better if we show some message e.g. (User does haves perssions on any queue) instead of showing blank line

        Show
        Karam Singh added a comment - After applying the latest patch, ran some : mapred. queue -showacls commands and found one issue and one minor enhancement : Issue -: In mapred-site.xml for queue acls if we specify user gorups instead of user queue -showacls command gives black list in console. E.g Have two queue: Q1,Q2 Q1 acl: SUBMIT_JOB=u1,u2 and ADMIN_JOB=U3 Q2 acls SUBMIT_JOB= UG1(where UG1 is user group) and ADMIN_JOB=U3 Now use commad: mapred queue -showacls (with user U5 who is part of UG1 group) It shows -: [ Queue acls for user : U5 ] Where U5 can submit job to Q2 Enhancemnet-: Consider same case and use comamnd : mapred queue -showacls (with user U4 who is not part of UG1 group). It shows -: [ Queue acls for user : U5 ] It would be better if we show some message e.g. (User does haves perssions on any queue) instead of showing blank line
        Hide
        eric baldeschwieler added a comment -

        When a user fails to submit a job, will the resulting error message give them enough info to determine which queues they can run against?

        As least an error message like: "Please run hadoop queue -showAcls to find the queues you have access to"

        Show
        eric baldeschwieler added a comment - When a user fails to submit a job, will the resulting error message give them enough info to determine which queues they can run against? As least an error message like: "Please run hadoop queue -showAcls to find the queues you have access to"
        Hide
        rahul k singh added a comment -

        Adding the new patch.
        Incorporated suggestion by eric.
        Changed the interface , this fix only returns the acls of the current user.
        Rectified the issue raised by karam w.r.t groups.

        Show
        rahul k singh added a comment - Adding the new patch. Incorporated suggestion by eric. Changed the interface , this fix only returns the acls of the current user. Rectified the issue raised by karam w.r.t groups.
        Hide
        rahul k singh added a comment -

        ran testpatch
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]

        Show
        rahul k singh added a comment - ran testpatch [exec] [exec] [exec] [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
        Hide
        Sreekanth Ramakrishnan added a comment -
        • Karam's comment about case where no user has access to any queue comment has not been handled. If user cannot submit to any queues which are configured, the appropriate message should be shown instead of blank line.
        • Correct JavaDoc in JobSubmissionProtocol for getCurrentUserQueueAclsInfo()
        • Error message in JobTracker.submitJob(JobID) can we change it to what Eric has commented?
        • In QueueManager getCurrentUserQueueAclsInfo() can we operationsAllowed create it and keep it instead of trying to create it lazily? would defintely improve readability of the code.
        • Minor nit the indendation in the getCurrentUserQueueAclsInfo() seems to be incorrect.
        • Why are we doing checking if user has access or not in getCurrentUserQueueAclsInfo()? Cant we make the code in the method much simpler? As follows
          for each queues:
          	for each operation:
          		allowed = hasAccess(queue, operation, ugi)
          		if(allowed)
          			add to operation allowed list
          	create queue acls info object based on operations allowed list.
          
        Show
        Sreekanth Ramakrishnan added a comment - Karam's comment about case where no user has access to any queue comment has not been handled. If user cannot submit to any queues which are configured, the appropriate message should be shown instead of blank line. Correct JavaDoc in JobSubmissionProtocol for getCurrentUserQueueAclsInfo() Error message in JobTracker.submitJob(JobID) can we change it to what Eric has commented? In QueueManager getCurrentUserQueueAclsInfo() can we operationsAllowed create it and keep it instead of trying to create it lazily? would defintely improve readability of the code. Minor nit the indendation in the getCurrentUserQueueAclsInfo() seems to be incorrect. Why are we doing checking if user has access or not in getCurrentUserQueueAclsInfo() ? Cant we make the code in the method much simpler? As follows for each queues: for each operation: allowed = hasAccess(queue, operation, ugi) if(allowed) add to operation allowed list create queue acls info object based on operations allowed list.
        Hide
        rahul k singh added a comment -

        added the comments mentioned above.

        Show
        rahul k singh added a comment - added the comments mentioned above.
        Hide
        Sreekanth Ramakrishnan added a comment -

        Patch looks fine to me.

        Show
        Sreekanth Ramakrishnan added a comment - Patch looks fine to me.
        Hide
        rahul k singh added a comment -

        test patch results:
        [exec] There appear to be 651 release audit warnings before the patch and 651 release audit warnings after applying the patch.
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] ======================================================================
        [exec] ======================================================================
        [exec] Finished build.
        [exec] ======================================================================
        [exec] ======================================================================

        Show
        rahul k singh added a comment - test patch results: [exec] There appear to be 651 release audit warnings before the patch and 651 release audit warnings after applying the patch. [exec] [exec] [exec] [exec] [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ======================================================================
        Hide
        Karam Singh added a comment -

        Message "Please run hadoop queue -showAcls to find the queues you have access to", is being printed in JobTracker Log, It should be printed on JobClient Side.
        As in trunk hadoop command is being for mapred operations, message shoud be changed to "Please run maped queue -showAcls to find the queues you have access to"

        Show
        Karam Singh added a comment - Message "Please run hadoop queue -showAcls to find the queues you have access to", is being printed in JobTracker Log, It should be printed on JobClient Side. As in trunk hadoop command is being for mapred operations, message shoud be changed to "Please run maped queue -showAcls to find the queues you have access to"
        Hide
        Hemanth Yamijala added a comment -

        I think the new patch uploaded is not complete. It seems to be missing QueueAclsInfo class. Also, the size is significantly lesser. Can you please check again ?

        Show
        Hemanth Yamijala added a comment - I think the new patch uploaded is not complete. It seems to be missing QueueAclsInfo class. Also, the size is significantly lesser. Can you please check again ?
        Hide
        rahul k singh added a comment -

        attaching the proper patch

        Show
        rahul k singh added a comment - attaching the proper patch
        Hide
        Hemanth Yamijala added a comment -

        Looks fine overall. Few minor comments and suggestions:

        • Indentation incorrect in a few places like displayQueueAclsInfoForCurrentUser.
        • Wrap to 80 characters at all places in the code.
        • Have a space between the opening brace of a block and the condition.
        • Message: "User <username> doesnt have access to any queue ": change doesnt to "does not". Terminate with a stop. So: User <username> does not have access to any queue.
        • Why do we need UnixUserGroupInformation.setCurrentUser(ugi); in getUGI(). I think we can use the conf defined in JobQueueClient to retrieve the current UGI information since the getUGI method saves it to this conf.
        • getCurrentUserQueueAclsInfo - I think a better name is getQueueAclsForCurrentUser(). This is inline with the other APIs defined.
        • versionId documentation in JobSubmissionProtocol has wrong method name.
        • JobTracker.submitJob() - The log statement should be included in the IOException as well so it can be printed to the client.
        • Javadoc: Gets the Queue ACLs for a user in the @return. Similarly in the comment in JobSubmissionProtocol.
        • QueueManager.getCurrentUserQueueAclsInfo - I think at this level it can be generic and make it QueueManager.getQueueAcls(UGI user). So tomorrow if there's a need to give for a specific user then we can use the same API.
        • In QueueAclsInfo, rather than use ArrayWritable, at other places in M/R, we write a count of the number of items, and then write each item. The elements are stored in a list that's created along with the object. This seems to be better as it does not need new objects to be created each time as is being done now.
        • I think it would be better to print the ACLs in a table format like:
          Queue Operations
          queuesdjkhdsfksjd1 submit-job,adminster-jobs
          queue2 submit-job
          and so on. Also, avoid space between the operations. This will help the output to work well with standard Unix like tools.
        • Test cases: I think we should add the current user to some of the queues and not set an arbitrary job ugi (like u1). You can look at TestQueueManager to see how we get get the Current user's UGI.
        • For qu1, why would we not check that both submit and administer jobs is available. Also, rather than checking for substring, I would verify that the entire string is available.
        • The group can be the current group for qu5, similar to current user for other queues
        • I think there must be another test in which the current user has no access to any queues and verify that an empty list is returned.
        Show
        Hemanth Yamijala added a comment - Looks fine overall. Few minor comments and suggestions: Indentation incorrect in a few places like displayQueueAclsInfoForCurrentUser. Wrap to 80 characters at all places in the code. Have a space between the opening brace of a block and the condition. Message: "User <username> doesnt have access to any queue ": change doesnt to "does not". Terminate with a stop. So: User <username> does not have access to any queue. Why do we need UnixUserGroupInformation.setCurrentUser(ugi); in getUGI(). I think we can use the conf defined in JobQueueClient to retrieve the current UGI information since the getUGI method saves it to this conf. getCurrentUserQueueAclsInfo - I think a better name is getQueueAclsForCurrentUser(). This is inline with the other APIs defined. versionId documentation in JobSubmissionProtocol has wrong method name. JobTracker.submitJob() - The log statement should be included in the IOException as well so it can be printed to the client. Javadoc: Gets the Queue ACLs for a user in the @return. Similarly in the comment in JobSubmissionProtocol. QueueManager.getCurrentUserQueueAclsInfo - I think at this level it can be generic and make it QueueManager.getQueueAcls(UGI user). So tomorrow if there's a need to give for a specific user then we can use the same API. In QueueAclsInfo, rather than use ArrayWritable, at other places in M/R, we write a count of the number of items, and then write each item. The elements are stored in a list that's created along with the object. This seems to be better as it does not need new objects to be created each time as is being done now. I think it would be better to print the ACLs in a table format like: Queue Operations queuesdjkhdsfksjd1 submit-job,adminster-jobs queue2 submit-job and so on. Also, avoid space between the operations. This will help the output to work well with standard Unix like tools. Test cases: I think we should add the current user to some of the queues and not set an arbitrary job ugi (like u1). You can look at TestQueueManager to see how we get get the Current user's UGI. For qu1, why would we not check that both submit and administer jobs is available. Also, rather than checking for substring, I would verify that the entire string is available. The group can be the current group for qu5, similar to current user for other queues I think there must be another test in which the current user has no access to any queues and verify that an empty list is returned.
        Hide
        Hemanth Yamijala added a comment -

        Regarding the documentation:

        Can you modify the command description as:

        Displays the queue name and associated queue operations allowed for the current user.The list consists of only those queues to which the user has access.

        Show
        Hemanth Yamijala added a comment - Regarding the documentation: Can you modify the command description as: Displays the queue name and associated queue operations allowed for the current user.The list consists of only those queues to which the user has access.
        Hide
        rahul k singh added a comment -

        Incorporated hemanth's comment

        Show
        rahul k singh added a comment - Incorporated hemanth's comment
        Hide
        rahul k singh added a comment -

        applying the new patch , previous 1 was malformed

        Show
        rahul k singh added a comment - applying the new patch , previous 1 was malformed
        Hide
        Hemanth Yamijala added a comment -

        Close, but a little more work:

        • In displayQueueAclsInfoForCurrentUser, when no ACLs are returned, we are using UserGroupInformation.getCurrentUGI().getUserName(). This could be null if the login is not done, right ?
        • I would recommend that the JobClient API be changed to getQueueAclsForCurrentUser, and the java doc updated accordingly.
        • QueueManager API need not be called getCurrentUserQueueAclsInfo, as we are passing a UGI. Let's just call it getQueueAcls(UGI ugi)
        • Javadoc for the QueueManager API is still saying 'current user' though we are passing in the UGI.
        • java.util.Iterator is not needed in QueueManager, I think, Can you check
        • I think the formatting of the QueueAclsInfo in the toString is not right. Let's keep all formatting in JobQueueClient API and remove the toString
        Show
        Hemanth Yamijala added a comment - Close, but a little more work: In displayQueueAclsInfoForCurrentUser, when no ACLs are returned, we are using UserGroupInformation.getCurrentUGI().getUserName(). This could be null if the login is not done, right ? I would recommend that the JobClient API be changed to getQueueAclsForCurrentUser, and the java doc updated accordingly. QueueManager API need not be called getCurrentUserQueueAclsInfo, as we are passing a UGI. Let's just call it getQueueAcls(UGI ugi) Javadoc for the QueueManager API is still saying 'current user' though we are passing in the UGI. java.util.Iterator is not needed in QueueManager, I think, Can you check I think the formatting of the QueueAclsInfo in the toString is not right. Let's keep all formatting in JobQueueClient API and remove the toString
        Hide
        rahul k singh added a comment -

        Incorporated hemanth's comments

        Show
        rahul k singh added a comment - Incorporated hemanth's comments
        Hide
        Hemanth Yamijala added a comment -

        Looks good. +1.

        Show
        Hemanth Yamijala added a comment - Looks good. +1.
        Hide
        Karam Singh added a comment -

        Applied hadoop-5419-5.patch.
        Found that when try to run command -: "hadoop queue -showacls" from a user who does not have access to any queue. It throes NPE : -
        Exception in thread "main" java.lang.NullPointerException
        at org.apache.hadoop.mapred.JobQueueClient.displayQueueAclsInfoForCurrentUser(JobQueueClient.java:162)
        at org.apache.hadoop.mapred.JobQueueClient.run(JobQueueClient.java:99)
        at org.apache.hadoop.util.ToolRunner.run(ToolRunner.java:65)
        at org.apache.hadoop.util.ToolRunner.run(ToolRunner.java:79)
        at org.apache.hadoop.mapred.JobQueueClient.main(JobQueueClient.java:182)

        Show
        Karam Singh added a comment - Applied hadoop-5419-5.patch. Found that when try to run command -: "hadoop queue -showacls" from a user who does not have access to any queue. It throes NPE : - Exception in thread "main" java.lang.NullPointerException at org.apache.hadoop.mapred.JobQueueClient.displayQueueAclsInfoForCurrentUser(JobQueueClient.java:162) at org.apache.hadoop.mapred.JobQueueClient.run(JobQueueClient.java:99) at org.apache.hadoop.util.ToolRunner.run(ToolRunner.java:65) at org.apache.hadoop.util.ToolRunner.run(ToolRunner.java:79) at org.apache.hadoop.mapred.JobQueueClient.main(JobQueueClient.java:182)
        Hide
        rahul k singh added a comment -

        Solves the NPE issue

        Show
        rahul k singh added a comment - Solves the NPE issue
        Hide
        rahul k singh added a comment -

        Unused ArrayWritable in QueueAclsInfo is removed so as to make it pass findbug.

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] ======================================================================
        [exec] ======================================================================
        [exec] Finished build.
        [exec] ======================================================================
        [exec] ======================================================================

        Show
        rahul k singh added a comment - Unused ArrayWritable in QueueAclsInfo is removed so as to make it pass findbug. [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ======================================================================
        Hide
        Hemanth Yamijala added a comment -

        Umm. I didn't understand the fix. If a user does not have access to any queue, an empty queue ACLs info list is passed to the client, and it prints the message saying this user has no ACLs or something similar. Where is the NPE coming from ? Can you explain ?

        Show
        Hemanth Yamijala added a comment - Umm. I didn't understand the fix. If a user does not have access to any queue, an empty queue ACLs info list is passed to the client, and it prints the message saying this user has no ACLs or something similar. Where is the NPE coming from ? Can you explain ?
        Hide
        rahul k singh added a comment -

        uploading the new patch.
        Removed changes for NPE issue, as that was due to hadoop-5419-4.patch.
        run test patch result is below:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 3 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]
        [exec]
        [exec]
        [exec]
        [exec] ======================================================================
        [exec] ======================================================================
        [exec] Finished build.
        [exec] ======================================================================
        [exec] ======================================================================
        [exec]
        [exec]

        Show
        rahul k singh added a comment - uploading the new patch. Removed changes for NPE issue, as that was due to hadoop-5419-4.patch. run test patch result is below: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] [exec] [exec] [exec] ====================================================================== [exec] ====================================================================== [exec] Finished build. [exec] ====================================================================== [exec] ====================================================================== [exec] [exec]
        Hide
        rahul k singh added a comment -

        ran "ant test" tests passed sucessfully.

        Show
        rahul k singh added a comment - ran "ant test" tests passed sucessfully.
        Hide
        Hemanth Yamijala added a comment -

        I committed this to trunk. Thanks, Rahul !

        Show
        Hemanth Yamijala added a comment - I committed this to trunk. Thanks, Rahul !
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #834 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/834/)
        . Provide a facility to query the Queue ACLs for the current user. Contributed by Rahul Kumar Singh.

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #834 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/834/ ) . Provide a facility to query the Queue ACLs for the current user. Contributed by Rahul Kumar Singh.
        Hide
        Robert Chansler added a comment -

        Attached example for earlier version not to be committed.

        Show
        Robert Chansler added a comment - Attached example for earlier version not to be committed.
        Hide
        Hemanth Yamijala added a comment -

        This patch fixes a test case path bug in the previous patch for the 20 branch (hadoop-5419-v20.patch). Not to be applied to the 20 branch though.

        Show
        Hemanth Yamijala added a comment - This patch fixes a test case path bug in the previous patch for the 20 branch (hadoop-5419-v20.patch). Not to be applied to the 20 branch though.

          People

          • Assignee:
            rahul k singh
            Reporter:
            Hemanth Yamijala
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development