Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-2836

Provide option to fail jobs when submitted to non-existent pools.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Component/s: contrib/fair-share
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In some environments, it might be desirable to explicitly specify the fair scheduler pools and to explicitly fail jobs that are not submitted to any of the pools.

      Current behavior of the fair scheduler is to submit jobs to a default pool if a pool name isn't specified or to create a pool with the new name if the pool name doesn't already exist. There should be a configuration option for the fair scheduler that causes it to noisily fail the job if it's submitted to a pool that isn't pre-specified or if the specified pool doesn't exist.

      1. MAPREDUCE-2836.patch
        8 kB
        Ahmed Radwan
      2. MAPREDUCE-2836_trunk.patch
        14 kB
        Ahmed Radwan
      3. MAPREDUCE-2836_trunk_rev4.patch
        15 kB
        Ahmed Radwan
      4. MAPREDUCE-2836_trunk_rev3.patch
        15 kB
        Ahmed Radwan
      5. MAPREDUCE-2836_trunk_rev2.patch
        14 kB
        Ahmed Radwan
      6. MAPREDUCE-2836_rev5.patch
        14 kB
        Ahmed Radwan
      7. MAPREDUCE-2836_rev4.patch
        12 kB
        Ahmed Radwan
      8. MAPREDUCE-2836_rev3.patch
        11 kB
        Ahmed Radwan
      9. MAPREDUCE-2836_rev2.patch
        11 kB
        Ahmed Radwan
      10. MAPREDUCE-2836_0.20_security.patch
        13 kB
        Ahmed Radwan
      11. MAPREDUCE-2836_0.20_security_rev3.patch
        14 kB
        Ahmed Radwan
      12. MAPREDUCE-2836_0.20_security_rev2.patch
        14 kB
        Ahmed Radwan

        Activity

        Hide
        Ahmed Radwan added a comment -

        This patch is against the "branch-0.20-security" branch.

        A new configuration property "mapred.fairscheuler.createpool" is added.

        To enable the modified behavior, this property should be explicitly set to false, otherwise, the old behavior remains the same (for backward compatibility).

        Added a new test case to test the modified behavior with job submission to default, existing and non-existing pools.

        Show
        Ahmed Radwan added a comment - This patch is against the "branch-0.20-security" branch. A new configuration property "mapred.fairscheuler.createpool" is added. To enable the modified behavior, this property should be explicitly set to false, otherwise, the old behavior remains the same (for backward compatibility). Added a new test case to test the modified behavior with job submission to default, existing and non-existing pools.
        Hide
        Matei Zaharia added a comment -

        Hi Ahmed,

        The patch looks like a good start, but there are a few issues with it:

        • There's a typo in the name "mapred.fairscheuler.createpool" (missing D in fairscheduler). Actually, I don't think the name "createpool" is great in general; maybe something like mapred.fairscheduler.allowUndeclaredPools would be more descriptive.
        • Throwing an IllegalArgumentException in a running jobtracker would crash the whole JobTracker, which is not what we want. There has to be some code, maybe in the job initialization listener, that catches the exception and calls fail() on the JobInProgress.
        • It's a little weird to be throwing and catching IllegalArgumentException as part of a normal code path. At the very least, create a new exception type (e.g. UndeclaredPoolException), but in fact it would be better to refactor the code so that it doesn't rely on throwing exceptions. For example, you could add a PoolManager.isValidPool(String name) method and call it on new jobs.
        Show
        Matei Zaharia added a comment - Hi Ahmed, The patch looks like a good start, but there are a few issues with it: There's a typo in the name "mapred.fairscheuler.createpool" (missing D in fairscheduler). Actually, I don't think the name "createpool" is great in general; maybe something like mapred.fairscheduler.allowUndeclaredPools would be more descriptive. Throwing an IllegalArgumentException in a running jobtracker would crash the whole JobTracker, which is not what we want. There has to be some code, maybe in the job initialization listener, that catches the exception and calls fail() on the JobInProgress. It's a little weird to be throwing and catching IllegalArgumentException as part of a normal code path. At the very least, create a new exception type (e.g. UndeclaredPoolException), but in fact it would be better to refactor the code so that it doesn't rely on throwing exceptions. For example, you could add a PoolManager.isValidPool(String name) method and call it on new jobs.
        Hide
        Ahmed Radwan added a comment -

        Many thanks Matei for your comments!

        I have updated the patch.

        • I agree, the "mapred.fairscheduler.allowUndeclaredPools" is a more descriptive property name, I changed it.
        • When a user tries to add a job to an undeclared pool, the exception thrown by PoolManager.addJob(..) will go through FairScheduler.JobListner.jobAdded(..), JobTracker.addJob(..), JobTracker.submitJob(..), JobClient.submitJobInternal(..), and finally thrown by Job.submit(..). This shouldn't cause the JobTracker to crash, but is a noisy way of communicating the problem.
        • I have also created a new exception UndeclaredPoolException instead of the IllegalArgumentException.
        Show
        Ahmed Radwan added a comment - Many thanks Matei for your comments! I have updated the patch. I agree, the "mapred.fairscheduler.allowUndeclaredPools" is a more descriptive property name, I changed it. When a user tries to add a job to an undeclared pool, the exception thrown by PoolManager.addJob(..) will go through FairScheduler.JobListner.jobAdded(..), JobTracker.addJob(..), JobTracker.submitJob(..), JobClient.submitJobInternal(..), and finally thrown by Job.submit(..). This shouldn't cause the JobTracker to crash, but is a noisy way of communicating the problem. I have also created a new exception UndeclaredPoolException instead of the IllegalArgumentException.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12491862/MAPREDUCE-2836_rev2.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/547//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/12491862/MAPREDUCE-2836_rev2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/547//console This message is automatically generated.
        Hide
        Matei Zaharia added a comment -

        Ah okay, that makes sense; I didn't realize that the exception would be propagated to the JobClient. If you've tested that and it works then this is fine. +1

        Show
        Matei Zaharia added a comment - Ah okay, that makes sense; I didn't realize that the exception would be propagated to the JobClient. If you've tested that and it works then this is fine. +1
        Hide
        Ahmed Radwan added a comment -

        Thanks Matei! Yes, The newly added and old unit tests run successfully. I have also fixed a minor typo in the javadoc of the PoolManager.getPool(..) method (@throws UndeclaredPoolException).

        Show
        Ahmed Radwan added a comment - Thanks Matei! Yes, The newly added and old unit tests run successfully. I have also fixed a minor typo in the javadoc of the PoolManager.getPool(..) method (@throws UndeclaredPoolException).
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12492505/MAPREDUCE-2836_rev3.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/576//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/12492505/MAPREDUCE-2836_rev3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/576//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        A few comments on the patch:

        • the new exception type needs a serialVersionUID
        • Rather than call conf.getBoolean() every time you need to look at this configuration, assign a member field in PoolManager (like is done for allocFile, etc)
        • I find the two different getPool() methods a little confusing. Perhaps you could do have the following methods:
          • public synchronized Pool getPool(String name): return the specified pool, throwing if it doesn't exist (the behavior when your 'create' param is false)
          • private synchronized Pool getOrCreatePool(String name): return the pool if it exists. If it doesn't exist, and the conf is true, create it and return it. If it doesn't exist and the conf is false, throw UndeclaredPoolException.
        • small nit: in the exception message for UndeclaredPoolException, put quotes around the pool name: "Pool name: '" + name + "' ..." – this makes it easier to debug if the user accidentally has some trailing whitespace causing a problem. You might consider having this message include something like "Valid pools are: " + StringUtils.join(", ", pools.keySet())

        Does that make sense?

        Show
        Todd Lipcon added a comment - A few comments on the patch: the new exception type needs a serialVersionUID Rather than call conf.getBoolean() every time you need to look at this configuration, assign a member field in PoolManager (like is done for allocFile, etc) I find the two different getPool() methods a little confusing. Perhaps you could do have the following methods: public synchronized Pool getPool(String name): return the specified pool, throwing if it doesn't exist (the behavior when your 'create' param is false) private synchronized Pool getOrCreatePool(String name): return the pool if it exists. If it doesn't exist, and the conf is true, create it and return it. If it doesn't exist and the conf is false, throw UndeclaredPoolException. small nit: in the exception message for UndeclaredPoolException, put quotes around the pool name: "Pool name: '" + name + "' ..." – this makes it easier to debug if the user accidentally has some trailing whitespace causing a problem. You might consider having this message include something like "Valid pools are: " + StringUtils.join(", ", pools.keySet()) Does that make sense?
        Hide
        Ahmed Radwan added a comment -

        Many thanks Todd for your comments!

        I have updated the patch.

        I have addressed your comments in the updated patch (attached). Regarding the getPool() methods, I agree it's a little confusing, but for compatibility I didn't want to change the signature of this public method. Based on your comment, I have modified the java docs for both methods to make their functionality clearer and avoid any confusion.

        Show
        Ahmed Radwan added a comment - Many thanks Todd for your comments! I have updated the patch. I have addressed your comments in the updated patch (attached). Regarding the getPool() methods, I agree it's a little confusing, but for compatibility I didn't want to change the signature of this public method. Based on your comment, I have modified the java docs for both methods to make their functionality clearer and avoid any confusion.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12492698/MAPREDUCE-2836_rev4.patch
        against trunk revision .

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

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

        +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 appears to introduce 8 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 ..

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//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/12492698/MAPREDUCE-2836_rev4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 appears to introduce 8 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 .. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/584//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Any idea what these new findbugs warnings are? The links above seem broken. I'll kick the test-patch build again, I think Giri fixed something this morning that was breaking these links.

        Show
        Todd Lipcon added a comment - Any idea what these new findbugs warnings are? The links above seem broken. I'll kick the test-patch build again, I think Giri fixed something this morning that was breaking these links.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12492698/MAPREDUCE-2836_rev4.patch
        against trunk revision .

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

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

        +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 appears to introduce 8 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 ..

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//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/12492698/MAPREDUCE-2836_rev4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 appears to introduce 8 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 .. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592//console This message is automatically generated.
        Hide
        Ahmed Radwan added a comment -

        Thanks Todd for kicking the test again!

        The links above are still broken (they are missing the "hadoop-mapreduce-project" after "trunk" in the url.

        I viewed the details from: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592/
        Then going through ->Build Artifacts -> View -> trunk/hadoop-mapreduce-project/patchprocess

        See for example: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592/artifact/trunk/hadoop-mapreduce-project/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html

        These Findbugs warnings are unrelated to the submitted patch in this ticket.

        Show
        Ahmed Radwan added a comment - Thanks Todd for kicking the test again! The links above are still broken (they are missing the "hadoop-mapreduce-project" after "trunk" in the url. I viewed the details from: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592/ Then going through ->Build Artifacts -> View -> trunk/hadoop-mapreduce-project/patchprocess See for example: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/592/artifact/trunk/hadoop-mapreduce-project/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html These Findbugs warnings are unrelated to the submitted patch in this ticket.
        Hide
        Todd Lipcon added a comment -

        Hi Ahmed,

        I took another detailed look at this and some potential issues.

        • In jobAdded(), it constructs a new JobInfo and puts it in the infos map before it calls poolMgr.addJob. So, in the case that an exception is thrown, the job will end up orphaned in the infos map.
        • this implementation has the bug that, if the allocations file is reloaded with one of the pools removed, users will still be allowed to submit jobs to it. That doesn't seem quite right.
        • in JobTracker.addJob, it looks like it just catches and warns if the jobAdded notification fails. It doesn't actually reject the job submission. I think your test doesn't catch this since it's using a fake JT and not the real implementation.
        • Maybe you can add a functional test that uses MiniMRCluster to see the above bug?

        One minor thing I noticed: we don't usually use camelCase in the configs. Perhaps mapred.fairscheduler.allow.undeclared.pools would be more consistent.

        I think I agree with Matei's earlier comment that it would be better to add a hook in the scheduler API like "checkJobSubmission(JobInProgress job)" that could be called after aclsManager.checkAccess and checkQueueValidity in submitJob. Then, to fix the other bug with pool reloading, add a new {Set<String> validPools which is loaded in reloadAllocs. At submission time you can simply check if the pool exists in that list. Then getPool() etc wouldn't need to be changed at all. Does that sound reasonable?

        Show
        Todd Lipcon added a comment - Hi Ahmed, I took another detailed look at this and some potential issues. In jobAdded(), it constructs a new JobInfo and puts it in the infos map before it calls poolMgr.addJob . So, in the case that an exception is thrown, the job will end up orphaned in the infos map. this implementation has the bug that, if the allocations file is reloaded with one of the pools removed, users will still be allowed to submit jobs to it. That doesn't seem quite right. in JobTracker.addJob, it looks like it just catches and warns if the jobAdded notification fails. It doesn't actually reject the job submission. I think your test doesn't catch this since it's using a fake JT and not the real implementation. Maybe you can add a functional test that uses MiniMRCluster to see the above bug? One minor thing I noticed: we don't usually use camelCase in the configs. Perhaps mapred.fairscheduler.allow.undeclared.pools would be more consistent. I think I agree with Matei's earlier comment that it would be better to add a hook in the scheduler API like "checkJobSubmission(JobInProgress job)" that could be called after aclsManager.checkAccess and checkQueueValidity in submitJob. Then, to fix the other bug with pool reloading, add a new { Set<String> validPools which is loaded in reloadAllocs . At submission time you can simply check if the pool exists in that list. Then getPool() etc wouldn't need to be changed at all. Does that sound reasonable?
        Hide
        Ahmed Radwan added a comment -

        Thanks Todd for you detailed comments.

        • In jobAdded(), it actually calls poolMgr.addJob(..) before constructing the JobInfo and putting it in the infos map (FairScheduler.java: lines 171 - 173). So in case of throwing an exception, no orphaned jobs will exist.
        • In case of reloading the allocation file, only new pool names are added, while existing ones will remain (even if removed from the allocation file). I thought this is the required behavior (i.e., only previously existing pools will get removed if the scheduler is restarted). I agree with you, I updated the patch to clear the pools map with every reload, so only pools in the reloaded file will exist.
        • The JobTracker.addJob(..) actually throws the exception (JobTracker.java: line 4030). If you mean the call to addJob (line 3985), the exception is caught, but it is logged and thrown again (line 3991).
        • I have modified the new test case to use the MiniMRCluster. The behavior remains the same; the exception propagates all the way to the JobClient.

        I have also modified the property name per your comment to not use camelCase.

        Show
        Ahmed Radwan added a comment - Thanks Todd for you detailed comments. In jobAdded(), it actually calls poolMgr.addJob(..) before constructing the JobInfo and putting it in the infos map (FairScheduler.java: lines 171 - 173). So in case of throwing an exception, no orphaned jobs will exist. In case of reloading the allocation file, only new pool names are added, while existing ones will remain (even if removed from the allocation file). I thought this is the required behavior (i.e., only previously existing pools will get removed if the scheduler is restarted). I agree with you, I updated the patch to clear the pools map with every reload, so only pools in the reloaded file will exist. The JobTracker.addJob(..) actually throws the exception (JobTracker.java: line 4030). If you mean the call to addJob (line 3985), the exception is caught, but it is logged and thrown again (line 3991). I have modified the new test case to use the MiniMRCluster. The behavior remains the same; the exception propagates all the way to the JobClient. I have also modified the property name per your comment to not use camelCase.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12493755/MAPREDUCE-2836_rev5.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/676//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/12493755/MAPREDUCE-2836_rev5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/676//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Hi Ahmed. Which branch are you looking at and testing on? The code I see in JobTracker.java is:

        
            synchronized (jobs) {
              synchronized (taskScheduler) {
                jobs.put(job.getProfile().getJobID(), job);
                for (JobInProgressListener listener : jobInProgressListeners) {
                  try {
                    listener.jobAdded(job);
                  } catch (IOException ioe) {
                    LOG.warn("Failed to add and so skipping the job : "
                        + job.getJobID() + ". Exception : " + ioe);
                  }
                }
              }
            }
        

        which would ignore the error.

        and in FairScheduler.java:

                eventLog.log("JOB_ADDED", job.getJobID());
                JobInfo info = new JobInfo(new JobSchedulable(FairScheduler.this, job, TaskType.MAP),
                    new JobSchedulable(FairScheduler.this, job, TaskType.REDUCE));
                infos.put(job, info);
                poolMgr.addJob(job); // Also adds job into the right PoolScheduable
                update();
        

        which would put into infos before calling poolMgr.

        Show
        Todd Lipcon added a comment - Hi Ahmed. Which branch are you looking at and testing on? The code I see in JobTracker.java is: synchronized (jobs) { synchronized (taskScheduler) { jobs.put(job.getProfile().getJobID(), job); for (JobInProgressListener listener : jobInProgressListeners) { try { listener.jobAdded(job); } catch (IOException ioe) { LOG.warn( "Failed to add and so skipping the job : " + job.getJobID() + ". Exception : " + ioe); } } } } which would ignore the error. and in FairScheduler.java: eventLog.log( "JOB_ADDED" , job.getJobID()); JobInfo info = new JobInfo( new JobSchedulable(FairScheduler. this , job, TaskType.MAP), new JobSchedulable(FairScheduler. this , job, TaskType.REDUCE)); infos.put(job, info); poolMgr.addJob(job); // Also adds job into the right PoolScheduable update(); which would put into infos before calling poolMgr.
        Hide
        Ahmed Radwan added a comment -

        I am using "branch-0.20-security" branch.

        Show
        Ahmed Radwan added a comment - I am using "branch-0.20-security" branch.
        Hide
        Todd Lipcon added a comment -

        The 0.20-security branch has a pretty out-of-date fair scheduler. Matei has proposed backporting the new scheduler to this branch. I think it's better to target the fair scheduler in trunk, assuming that it will be backported to 0.20 (eg in CDH it already is)

        Show
        Todd Lipcon added a comment - The 0.20-security branch has a pretty out-of-date fair scheduler. Matei has proposed backporting the new scheduler to this branch. I think it's better to target the fair scheduler in trunk, assuming that it will be backported to 0.20 (eg in CDH it already is)
        Hide
        Ahmed Radwan added a comment -

        Makes sense, attaching a new patch against trunk.

        Show
        Ahmed Radwan added a comment - Makes sense, attaching a new patch against 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/12493951/MAPREDUCE-2836_trunk.patch
        against trunk revision .

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

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

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//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/12493951/MAPREDUCE-2836_trunk.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-hs.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-shuffle.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-app.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-jobclient.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-resourcemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/693//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Hey Ahmed. Thanks for the revision. A few notes on the latest patch:

        • you can make the allowUndeclaredPools member final, since it's set in the constructor
        • rather than making PoolManager.declaredPools public, add an accessor for it
        • the declaredPools set needs to be synchronized more carefully:
          • since you're exposing the Set<String> via a getter, it's probably better to make a new instance every time you reload the pools. That way, the getter returns just a snapshot that won't change. For example, you could do:
            this.declaredPools = Collections.unmodifiableSet(new TreeSet<String>(poolNamesInAllocFile));
            

            and then make the getter synchronized. This makes it clear that the contents of that set won't change, and you can expose it to other classes safely. The issue with the current implementation is that a job may be submitted just as the set is being modified, and you'd get a spurious job rejection.

        • nit: consider using a TreeSet instead of HashSet, so that the pool names are sorted in the error message
        Show
        Todd Lipcon added a comment - Hey Ahmed. Thanks for the revision. A few notes on the latest patch: you can make the allowUndeclaredPools member final, since it's set in the constructor rather than making PoolManager.declaredPools public, add an accessor for it the declaredPools set needs to be synchronized more carefully: since you're exposing the Set<String> via a getter, it's probably better to make a new instance every time you reload the pools. That way, the getter returns just a snapshot that won't change. For example, you could do: this .declaredPools = Collections.unmodifiableSet( new TreeSet< String >(poolNamesInAllocFile)); and then make the getter synchronized. This makes it clear that the contents of that set won't change, and you can expose it to other classes safely. The issue with the current implementation is that a job may be submitted just as the set is being modified, and you'd get a spurious job rejection. nit: consider using a TreeSet instead of HashSet, so that the pool names are sorted in the error message
        Hide
        Ahmed Radwan added a comment -

        Many thanks Todd for highlighting these points! I am Attaching an updated patch per your comments.

        A couple of notes:

        • allowUndeclaredPools is set with the other conf properties in start(), and not in the constructor, so I kept it non-final.
        • I have incorporated the synchronization changes you proposed in addition to using TreeSet instead of HashSet.
        Show
        Ahmed Radwan added a comment - Many thanks Todd for highlighting these points! I am Attaching an updated patch per your comments. A couple of notes: allowUndeclaredPools is set with the other conf properties in start(), and not in the constructor, so I kept it non-final. I have incorporated the synchronization changes you proposed in addition to using TreeSet instead of HashSet.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494202/MAPREDUCE-2836_trunk_rev2.patch
        against trunk revision .

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

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

        +1 javadoc. The javadoc tool did not generate any warning messages.

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

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

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

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/713//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/713//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/12494202/MAPREDUCE-2836_trunk_rev2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/713//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/713//console This message is automatically generated.
        Hide
        Ahmed Radwan added a comment -

        Since the backport patch for MR-2981 is committed, I am attaching the patch for the "branch-0.20-security" branch.

        Show
        Ahmed Radwan added a comment - Since the backport patch for MR-2981 is committed, I am attaching the patch for the "branch-0.20-security" branch.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494511/MAPREDUCE-2836_0.20_security.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/739//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/12494511/MAPREDUCE-2836_0.20_security.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/739//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -
        +  public final static String ALLOW_UNDECLARED_POOLS_KEY =
        +    "mapred.fairscheduler.allow.undeclared.pools";   // A configuration property
        +    // that controls the ability of submitting jobs to pools not declared in the
        +    // scheduler allocation file.
        +    private boolean allowUndeclaredPools = false;
        

        Please convert the comment describing this variable to be a javadoc comment that proceeds the declaration.
        Also, indentation is messed up on allowUndeclaredPools

        +  private Boolean submitJob(String pool) throws IOException {
        

        The result of this function is never used, so may as well make it void. Or, perhaps it should return boolean (unboxed) and then the callers should assert that the job was successful in testValidPoolName

        +    if (pool != null)
        +      conf.set(POOL_PROPERTY, pool);
        

        Style nit: add braces

        +      } catch (IOException ioe){
        +        LOG.error("Problem in job submission: " + ioe);
        +        throw ioe;
        

        Would be nice if the log message here included the job ID.

        +   * Subclasses can override to provide any scheduler-specific checking
        +   * mechanism for job submission.
        +   * @param job
        +   * @return
        +   * @throws IOException
        

        No need for empty @return in the javadoc, since it's void.

        +public class TestFairSchedulerPoolNames extends TestCase {
        

        Should use JUnit 4 style test case - ie don't extend anything, then annotate the test methods with @Before, @After, and @Test.

        Should also update src/docs/src/documentation/content/xdocs/fair_scheduler.xml to include the new parameter in the documentation, perhaps under the "Scheduler parameters in mapred-site.xml" section

        Show
        Todd Lipcon added a comment - + public final static String ALLOW_UNDECLARED_POOLS_KEY = + "mapred.fairscheduler.allow.undeclared.pools" ; // A configuration property + // that controls the ability of submitting jobs to pools not declared in the + // scheduler allocation file. + private boolean allowUndeclaredPools = false ; Please convert the comment describing this variable to be a javadoc comment that proceeds the declaration. Also, indentation is messed up on allowUndeclaredPools + private Boolean submitJob( String pool) throws IOException { The result of this function is never used, so may as well make it void . Or, perhaps it should return boolean (unboxed) and then the callers should assert that the job was successful in testValidPoolName + if (pool != null ) + conf.set(POOL_PROPERTY, pool); Style nit: add braces + } catch (IOException ioe){ + LOG.error( "Problem in job submission: " + ioe); + throw ioe; Would be nice if the log message here included the job ID. + * Subclasses can override to provide any scheduler-specific checking + * mechanism for job submission. + * @param job + * @ return + * @ throws IOException No need for empty @return in the javadoc, since it's void. + public class TestFairSchedulerPoolNames extends TestCase { Should use JUnit 4 style test case - ie don't extend anything, then annotate the test methods with @Before, @After, and @Test. Should also update src/docs/src/documentation/content/xdocs/fair_scheduler.xml to include the new parameter in the documentation, perhaps under the "Scheduler parameters in mapred-site.xml" section
        Hide
        Ahmed Radwan added a comment -

        Attaching updated patches for trunk and branch-0.20-security.

        Thanks!

        Show
        Ahmed Radwan added a comment - Attaching updated patches for trunk and branch-0.20-security. Thanks!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494590/MAPREDUCE-2836_0.20_security_rev2.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/749//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/12494590/MAPREDUCE-2836_0.20_security_rev2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/749//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Two tiny nits:

        • the indentation is still off on the declaration of allowUndeclaredPools
        • for JUnit 4, assertions come from the org.junit.Assert package - you can do:
          import static org.junit.Assert.*;
          

          in order to get all of them imported

        Otherwise looks great!

        Show
        Todd Lipcon added a comment - Two tiny nits: the indentation is still off on the declaration of allowUndeclaredPools for JUnit 4, assertions come from the org.junit.Assert package - you can do: import static org.junit.Assert.*; in order to get all of them imported Otherwise looks great!
        Hide
        Ahmed Radwan added a comment -

        Updated patches. Thanks Todd.

        Show
        Ahmed Radwan added a comment - Updated patches. Thanks Todd.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12494864/MAPREDUCE-2836_0.20_security_rev3.patch
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/771//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/12494864/MAPREDUCE-2836_0.20_security_rev3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/771//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        +1. Running test-patch now. I'll commit to 20S and trunk assuming test-patch is clean.

        Show
        Todd Lipcon added a comment - +1. Running test-patch now. I'll commit to 20S and trunk assuming test-patch is clean.
        Hide
        Todd Lipcon added a comment -

        [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 5 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 (version 1.3.8) warnings.

        Show
        Todd Lipcon added a comment - [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 5 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 (version 1.3.8) warnings.
        Hide
        Todd Lipcon added a comment -

        Committed to trunk and 0.20-security. Though the fair scheduler isn't used in MR2/trunk, the trunk commit will hopefully make it easier if anyone wants to backport this to 0.22 or 0.21.

        Show
        Todd Lipcon added a comment - Committed to trunk and 0.20-security. Though the fair scheduler isn't used in MR2/trunk, the trunk commit will hopefully make it easier if anyone wants to backport this to 0.22 or 0.21.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #902 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/902/)
        MAPREDUCE-2836. Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #902 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/902/ ) MAPREDUCE-2836 . Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #979 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/979/)
        MAPREDUCE-2836. Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #979 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/979/ ) MAPREDUCE-2836 . Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #917 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/917/)
        MAPREDUCE-2836. Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #917 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/917/ ) MAPREDUCE-2836 . Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #803 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/803/)
        MAPREDUCE-2836. Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #803 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/803/ ) MAPREDUCE-2836 . Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #833 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/833/)
        MAPREDUCE-2836. Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #833 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/833/ ) MAPREDUCE-2836 . Provide option to fail jobs when submitted to non-existent fair scheduler pools. Contributed by Ahmed Radwan. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171832 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/FairScheduler.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/PoolManager.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/java/org/apache/hadoop/mapred/UndeclaredPoolException.java /hadoop/common/trunk/hadoop-mapreduce-project/src/contrib/fairscheduler/src/test/org/apache/hadoop/mapred/TestFairSchedulerPoolNames.java /hadoop/common/trunk/hadoop-mapreduce-project/src/docs/src/documentation/content/xdocs/fair_scheduler.xml /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/JobTracker.java /hadoop/common/trunk/hadoop-mapreduce-project/src/java/org/apache/hadoop/mapred/TaskScheduler.java
        Hide
        Matt Foley added a comment -

        Closed upon release of Hadoop-1.1.0.

        Show
        Matt Foley added a comment - Closed upon release of Hadoop-1.1.0.

          People

          • Assignee:
            Ahmed Radwan
            Reporter:
            Jeff Bean
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development