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

JobClient poll intervals should be job configurations, not cluster configurations

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.21.0
    • Fix Version/s: None
    • Component/s: client
    • Labels:
      None

      Description

      Job.waitForCompletion gets the poll interval from the Cluster object's configuration rather than its own Job configuration. This is counter-intuitive - Chris and I both made this same mistake working on MAPREDUCE-64, and Aaron agrees as well.

        Activity

        Hide
        Todd Lipcon added a comment -

        Seems fine by me, Chris. Thanks for taking a look.

        Show
        Todd Lipcon added a comment - Seems fine by me, Chris. Thanks for taking a look.
        Hide
        Chris Douglas added a comment -

        Do we need to maintain compatibility on these static functions, since they're a public API?

        I'd say no. They were added in 0.21, which hasn't been released. It can be patched in both.

        Maintaining the separation between "progress" and "completion" polling intervals offers very little in functionality. Any objections to collapsing these tunables into a single set method on the Job instance?

        Show
        Chris Douglas added a comment - Do we need to maintain compatibility on these static functions, since they're a public API? I'd say no. They were added in 0.21, which hasn't been released. It can be patched in both. Maintaining the separation between "progress" and "completion" polling intervals offers very little in functionality. Any objections to collapsing these tunables into a single set method on the Job instance?
        Hide
        Todd Lipcon added a comment -

        If polling intervals are job-level configuration parameters, Job. getCompletionPollInterval(conf) and Job.getProgressPollInterval(conf) should be not static methods and should not take configuration as the parameter. The methods should read the values from Job's conf directly.

        OK. Do we need to maintain compatibility on these static functions, since they're a public API? (eg mark the static ones deprecated, then make non-static ones that forward to the static ones for now)

        Show
        Todd Lipcon added a comment - If polling intervals are job-level configuration parameters, Job. getCompletionPollInterval(conf) and Job.getProgressPollInterval(conf) should be not static methods and should not take configuration as the parameter. The methods should read the values from Job's conf directly. OK. Do we need to maintain compatibility on these static functions, since they're a public API? (eg mark the static ones deprecated, then make non-static ones that forward to the static ones for now)
        Hide
        Amareshwari Sriramadasu added a comment -

        If polling intervals are job-level configuration parameters, Job. getCompletionPollInterval(conf) and Job.getProgressPollInterval(conf) should be not static methods and should not take configuration as the parameter. The methods should read the values from Job's conf directly.

        Shall we add setters also in Job to be consistent with other getters and setters?

        Show
        Amareshwari Sriramadasu added a comment - If polling intervals are job-level configuration parameters, Job. getCompletionPollInterval(conf) and Job.getProgressPollInterval(conf) should be not static methods and should not take configuration as the parameter. The methods should read the values from Job's conf directly. Shall we add setters also in Job to be consistent with other getters and setters?
        Hide
        Aaron Kimball added a comment -

        Based on my understanding of the different conf objects, +1.

        Show
        Aaron Kimball added a comment - Based on my understanding of the different conf objects, +1.
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422585/mapreduce-1120.txt
        against trunk revision 826767.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

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

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/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/12422585/mapreduce-1120.txt against trunk revision 826767. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/81/console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        Trivial patch. To test, I ran TestTaskFail which uses this functionality, and verified that it's still polling quickly instead of sleeping (that testcase was setting the config in both the job and the cluster)

        Show
        Todd Lipcon added a comment - Trivial patch. To test, I ran TestTaskFail which uses this functionality, and verified that it's still polling quickly instead of sleeping (that testcase was setting the config in both the job and the cluster)

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development