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

Memory management variables need a backwards compatibility option after HADOOP-5881

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.1
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Fixed backwards compatibility by re-introducing and deprecating removed memory monitoring related configuration options.

      Description

      HADOOP-5881 modified variables related to memory management without looking at the backwards compatibility angle. This JIRA is to adress the gap. Marking it a blocker for 0.20.1

      1. hadoop-5919-1.patch
        29 kB
        rahul k singh
      2. hadoop-5919-2.patch
        38 kB
        rahul k singh
      3. hadoop-5919-3.patch
        39 kB
        rahul k singh
      4. hadoop-5919-4.patch
        26 kB
        rahul k singh
      5. hadoop-5919-5.patch
        29 kB
        rahul k singh
      6. hadoop-5919-6.patch
        29 kB
        rahul k singh
      7. hadoop-5919-7.patch
        19 kB
        rahul k singh
      8. hadoop-5919-8.patch
        22 kB
        rahul k singh
      9. hadoop-5919-9.patch
        22 kB
        rahul k singh
      10. hadoop-5919-10.patch
        23 kB
        rahul k singh
      11. hadoop-5919-11.patch
        24 kB
        rahul k singh
      12. hadoop-5919-12.patch
        26 kB
        rahul k singh
      13. hadoop-5919-12-20.patch
        26 kB
        rahul k singh
      14. hadoop-5919-13.patch
        25 kB
        rahul k singh
      15. hadoop-5919-13-20.patch
        25 kB
        rahul k singh
      16. hadoop-5919-14.patch
        26 kB
        rahul k singh
      17. hadoop-5919-14-20.patch
        26 kB
        rahul k singh
      18. hadoop-5919-15.patch
        27 kB
        rahul k singh
      19. hadoop-5919-15-20.patch
        27 kB
        rahul k singh

        Issue Links

          Activity

          Hide
          Hemanth Yamijala added a comment -

          In a brief discussion I had with Owen, he suggested that we use this patch as an opportunity to provide a facility in the configuration API to handle deprecated configuration variables.

          The basic idea is to have a Map<String, String[]> in the Configuration class that would be statically defined and populated. The key is the deprecated config, and the value is a list of replacement configs (in plural because there are instances as in HADOOP-5881, where a single key is split into two for e.g. memory-per-slot into memory-per-map-slot and memory-per-reduce-slot).

          When a get is done on a deprecated key, this Configuration API will print a deprecation warning, and return the value of the first string in the list.

          When a set is done, likewise, the API will print a deprecation warning and set all the values in the replacement keys.

          In cases where there is no replacement, we could print an ERROR message and return null maybe, or throw an exception.

          Thoughts ?

          Show
          Hemanth Yamijala added a comment - In a brief discussion I had with Owen, he suggested that we use this patch as an opportunity to provide a facility in the configuration API to handle deprecated configuration variables. The basic idea is to have a Map<String, String[]> in the Configuration class that would be statically defined and populated. The key is the deprecated config, and the value is a list of replacement configs (in plural because there are instances as in HADOOP-5881 , where a single key is split into two for e.g. memory-per-slot into memory-per-map-slot and memory-per-reduce-slot). When a get is done on a deprecated key, this Configuration API will print a deprecation warning, and return the value of the first string in the list. When a set is done, likewise, the API will print a deprecation warning and set all the values in the replacement keys. In cases where there is no replacement, we could print an ERROR message and return null maybe, or throw an exception. Thoughts ?
          Hide
          eric baldeschwieler added a comment -

          You are suggesting that it is possible to have an empty list of new variables to map to the old.

          In this case a set should be a noop that prints a WARNING, correct?

          Show
          eric baldeschwieler added a comment - You are suggesting that it is possible to have an empty list of new variables to map to the old. In this case a set should be a noop that prints a WARNING, correct?
          Hide
          Hemanth Yamijala added a comment -

          In some more discussion, we found some problems in the proposed approach. Primarily, two problems:

          • If the list of deprecated configurations is centralized in the Configuration class, what happens after project split ? So, if I have to deprecate a configuration of mapred, should I submit a patch to core ? And since theoretically they can have different release cycles, would that not conflict with requirements of different projects.
          • Another problem is that the mapping as proposed above seems simplistic for some cases. For e.g. consider the split of number of slots into map slots and reduce slots that happened a couple of releases back. If some change like that needs to be supported, just setting the same value to both map slots and reduce slots seems incorrect. A better mapping could be to split half of them into map slots and half into reduce slots. In other words, it seems like we may need a more complex mapping mechanism.

          Given all these, I am thinking this should definitely be the subject on another JIRA, as people might have more comments on the approach.

          And since this bug is a blocker for 0.20.1, I suggest we do a mapping specifically in the relevant classes for this bug (the memory related classes) and move over to using a centralized framework for deprecating configurations once the configuration JIRA is made available.

          Does this make sense ?

          Show
          Hemanth Yamijala added a comment - In some more discussion, we found some problems in the proposed approach. Primarily, two problems: If the list of deprecated configurations is centralized in the Configuration class, what happens after project split ? So, if I have to deprecate a configuration of mapred, should I submit a patch to core ? And since theoretically they can have different release cycles, would that not conflict with requirements of different projects. Another problem is that the mapping as proposed above seems simplistic for some cases. For e.g. consider the split of number of slots into map slots and reduce slots that happened a couple of releases back. If some change like that needs to be supported, just setting the same value to both map slots and reduce slots seems incorrect. A better mapping could be to split half of them into map slots and half into reduce slots. In other words, it seems like we may need a more complex mapping mechanism. Given all these, I am thinking this should definitely be the subject on another JIRA, as people might have more comments on the approach. And since this bug is a blocker for 0.20.1, I suggest we do a mapping specifically in the relevant classes for this bug (the memory related classes) and move over to using a centralized framework for deprecating configurations once the configuration JIRA is made available. Does this make sense ?
          Hide
          Hemanth Yamijala added a comment -

          Discussed with Owen on the issues.

          We think the project split is a knotty issue and may need a resolution that merits further discussion in a separate JIRA. But we do have a solution that we think will work. Hence for the purpose of this jira, we are still proposing to go ahead with introducing the deprecated mapping. And handle the project split issue in a follow-up jira, maybe for Hadoop 0.21.

          Regarding the second point of a more complicated mapping, again we agree that valid use cases for this could exist. We think it is not necessary for the deprecation map approach to handle every single deprecation in configuration. In other words, if a complicated use case exists, it could be done by the application itself - outside the configuration API. Or an extension to the deprecation mechanism can also be looked at as a further enhancement.

          Show
          Hemanth Yamijala added a comment - Discussed with Owen on the issues. We think the project split is a knotty issue and may need a resolution that merits further discussion in a separate JIRA. But we do have a solution that we think will work. Hence for the purpose of this jira, we are still proposing to go ahead with introducing the deprecated mapping. And handle the project split issue in a follow-up jira, maybe for Hadoop 0.21. Regarding the second point of a more complicated mapping, again we agree that valid use cases for this could exist. We think it is not necessary for the deprecation map approach to handle every single deprecation in configuration. In other words, if a complicated use case exists, it could be done by the application itself - outside the configuration API. Or an extension to the deprecation mechanism can also be looked at as a further enhancement.
          Hide
          rahul k singh added a comment - - edited

          All physical memory related stuff have been removed.

          mapred.task.default.maxvmem => removed completely
          mapred.tasktracker.vmem.reserved ==> this key is removed.Admin can attain similar behaviour using keys mapred.cluster.map.memory.mb and mapred.cluster.map.memory.mb.

          mapred.task.limit.maxvmem ==> split into mapred.cluster.max.map.memory.mb mapred.cluster.max.reduce.memory.mb
          for example if you set "mapred.task.limit.maxvmem" = 10 , then "mapred.cluster.max.map.memory.mb" = 10/1024. same with reduce.

          mapred.task.maxvmem ==> split into mapred.job.map.memory.mb,mapred.job.reduce.memory.mb. The set behaviour would be same
          as "mapred.task.limit.maxvmem"

          Design.
          Assumption:

          1. Incase there is 1-1 mapping of the key , the value of the deprecated key is applied to the new key.

          Data Structures.

          deprecatedKeys<String,DeprecatedKeyMapping>
          Skeleton of DeprecatedKeyMapping
          
          static class DeprecatedKeyMapping{
          
             String[] keyMappings;
             String customMessage;
          }
          

          for keys which are removed , keyMappings would be null and keys would have only custom message.

          while setting the conf , if there is 1-1 mapping ,Configuration would take care of that , incase of multiple values for a deprecated key , respective code paths would be setting the value.

          Show
          rahul k singh added a comment - - edited All physical memory related stuff have been removed. mapred.task.default.maxvmem => removed completely mapred.tasktracker.vmem.reserved ==> this key is removed.Admin can attain similar behaviour using keys mapred.cluster.map.memory.mb and mapred.cluster.map.memory.mb. mapred.task.limit.maxvmem ==> split into mapred.cluster.max.map.memory.mb mapred.cluster.max.reduce.memory.mb for example if you set "mapred.task.limit.maxvmem" = 10 , then "mapred.cluster.max.map.memory.mb" = 10/1024. same with reduce. mapred.task.maxvmem ==> split into mapred.job.map.memory.mb,mapred.job.reduce.memory.mb. The set behaviour would be same as "mapred.task.limit.maxvmem" Design. Assumption: 1. Incase there is 1-1 mapping of the key , the value of the deprecated key is applied to the new key. Data Structures. deprecatedKeys< String ,DeprecatedKeyMapping> Skeleton of DeprecatedKeyMapping static class DeprecatedKeyMapping{ String [] keyMappings; String customMessage; } for keys which are removed , keyMappings would be null and keys would have only custom message. while setting the conf , if there is 1-1 mapping ,Configuration would take care of that , incase of multiple values for a deprecated key , respective code paths would be setting the value.
          Hide
          rahul k singh added a comment -

          attaching the first patch with comments.

          Show
          rahul k singh added a comment - attaching the first patch with comments.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Looked at the patch. Comments:

          General:

          • After class/function names, there should be a space before "{" or "(" or ","
          • Some lines are longer than 80 char boundary. Wrap them around.

          Configuration.DeprecatedKeyMapping

          • can be private.
          • Fix the javadoc for this. Move the comments from isDeprecatedKey to here.
          • We can treat an empty array value the same way we treat a null value
          • keymappings and message should be initialized
          • Put some documentation before the static block for the deprecated keys as to what kind of entries can be present.
          • A single NoLongerUsedKey object can be used instead of repetitive new DeprecatedKeyMapping(null,"The key is no longer used")
          • In the messages, reference should be made to other keys in cases where keys are no longer used.
          • isDeprecatedKey name is inappropriate. It should be something in the lines of processDeprecatedKey.
          • isDeprecatedKey can be simplified like the following:
                  if (mapping.getKeyMappings() != null) {
                    for (String newKey : mapping.getKeyMappings()) {
                      properties.setProperty(newKey, value);
                    }
                  }
                  LOG.warn(mapping.getMessage());
                  return true;
               

          Configuration.set():

          • needs to set variables to the overlay too. We need a test case for this specific scenario.

          Configuratin.get()

          • document conf.get() method regarding the treatment of deprecated keys
          • put the log statement in a {} block. In general, we put even single line conditional statements in a {} block.

          Configuration.getRaw(), Configuration.get(String,String):

          • These also need the deprecated variables code.
          • I think the code should be in a common place.
          • Correspondingly, the javadoc for these two methods also needs to be fixed.

          JobConf:

          • The four methods {get|set}

            MemoryFor

            {Map|Reduce}

            Task() need to be public. It was missed in HADOOP-5881.

          • MAPRED_TASK_MAXVMEM_PROPERTY, UPPER_LIMIT_ON_TASK_VMEM_PROPERTY should be deprecated with appropriate references in javadoc to what instead has to be used.
          • The deprecated methods should also carry the same/similar javadoc messages.
          • Remove useless empty log warn messages in the deprecated methods
          • getMaxVirtualMemoryForTask()
            If one of the new parameters for map or reduce is missing, the job is anyway not accepted. So the logic in this method has to change.
          • getMemoryForMapTask and getMemoryForReduceTask
            • Remove/correct log messages
            • val>0 && val<1 check will never happen. It should actually be done by converting values into float/double
            • The logic can be simplified by checking for new values first. Also, a separate null check for old values is not needed.
            • Add documentation for the old variables and the old methods.

          CapacityTaskScheduler.initializeMemoryConf

          • Separate null checks for the values is not needed.
          • val>0 && val<1 check will never happen. It should actually be done by converting values into float/double

          JobTracker.initializeTaskMemoryRelatedConfig()

          • Changes are also needed in this method when some job has old values.

          Haven't looked at the testcases yet, will look at them in the next iteration.

          Show
          Vinod Kumar Vavilapalli added a comment - Looked at the patch. Comments: General: After class/function names, there should be a space before "{" or "(" or "," Some lines are longer than 80 char boundary. Wrap them around. Configuration.DeprecatedKeyMapping can be private. Fix the javadoc for this. Move the comments from isDeprecatedKey to here. We can treat an empty array value the same way we treat a null value keymappings and message should be initialized Put some documentation before the static block for the deprecated keys as to what kind of entries can be present. A single NoLongerUsedKey object can be used instead of repetitive new DeprecatedKeyMapping(null,"The key is no longer used") In the messages, reference should be made to other keys in cases where keys are no longer used. isDeprecatedKey name is inappropriate. It should be something in the lines of processDeprecatedKey. isDeprecatedKey can be simplified like the following: if (mapping.getKeyMappings() != null ) { for ( String newKey : mapping.getKeyMappings()) { properties.setProperty(newKey, value); } } LOG.warn(mapping.getMessage()); return true ; Configuration.set(): needs to set variables to the overlay too. We need a test case for this specific scenario. Configuratin.get() document conf.get() method regarding the treatment of deprecated keys put the log statement in a {} block. In general, we put even single line conditional statements in a {} block. Configuration.getRaw(), Configuration.get(String,String): These also need the deprecated variables code. I think the code should be in a common place. Correspondingly, the javadoc for these two methods also needs to be fixed. JobConf: The four methods {get|set} MemoryFor {Map|Reduce} Task() need to be public. It was missed in HADOOP-5881 . MAPRED_TASK_MAXVMEM_PROPERTY, UPPER_LIMIT_ON_TASK_VMEM_PROPERTY should be deprecated with appropriate references in javadoc to what instead has to be used. The deprecated methods should also carry the same/similar javadoc messages. Remove useless empty log warn messages in the deprecated methods getMaxVirtualMemoryForTask() If one of the new parameters for map or reduce is missing, the job is anyway not accepted. So the logic in this method has to change. getMemoryForMapTask and getMemoryForReduceTask Remove/correct log messages val>0 && val<1 check will never happen. It should actually be done by converting values into float/double The logic can be simplified by checking for new values first. Also, a separate null check for old values is not needed. Add documentation for the old variables and the old methods. CapacityTaskScheduler.initializeMemoryConf Separate null checks for the values is not needed. val>0 && val<1 check will never happen. It should actually be done by converting values into float/double JobTracker.initializeTaskMemoryRelatedConfig() Changes are also needed in this method when some job has old values. Haven't looked at the testcases yet, will look at them in the next iteration.
          Hide
          rahul k singh added a comment -

          Attached new patch.

          Show
          rahul k singh added a comment - Attached new patch.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          We need new patches to accomodate the project split.

          Show
          Vinod Kumar Vavilapalli added a comment - We need new patches to accomodate the project split.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I am going to generate new patches myself and start reviewing them.

          Show
          Vinod Kumar Vavilapalli added a comment - I am going to generate new patches myself and start reviewing them.
          Hide
          rahul k singh added a comment -

          After discussion with Hemanth and vinod , we have decided that this patch would only handle the case for the Memory management variables backward specifically.

          Unlike the earlier thought(Having a deprecated key map design , explained by hemanth in earlier updates) , we are just handing the specific case.

          The is being done as the components are split and we need more code change to handle the configurations across multiple components, hence it should be handled independently.

          Show
          rahul k singh added a comment - After discussion with Hemanth and vinod , we have decided that this patch would only handle the case for the Memory management variables backward specifically. Unlike the earlier thought(Having a deprecated key map design , explained by hemanth in earlier updates) , we are just handing the specific case. The is being done as the components are split and we need more code change to handle the configurations across multiple components, hence it should be handled independently.
          Hide
          Hemanth Yamijala added a comment -

          After discussion with Hemanth and vinod , we have decided that this patch would only handle the case for the Memory management variables backward specifically.

          Specifically, we are only decoupling the two jiras, because they need to anyway now. I filed HADOOP-6105 to take the changes in configuration forward.

          Show
          Hemanth Yamijala added a comment - After discussion with Hemanth and vinod , we have decided that this patch would only handle the case for the Memory management variables backward specifically. Specifically, we are only decoupling the two jiras, because they need to anyway now. I filed HADOOP-6105 to take the changes in configuration forward.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Looked at the latest patch. Some comments:

          CapacityTaskScheduler.java

          • initializeMemoryRelatedConf: New values don't take precedence with the current code
          • Add a separate test-case for the above method in TestCapacityScheduler w.r.t deprecated values.

          JobTracker.java

          • same as the above changes in CapacityTaskScheduler.java

          JobConf:

          • getMaxVirtualMemoryForTask():
            • If new keys are present, we should return the maximum of the new values specified for map/reduce tasks
            • Directly call getMemoryForMapTask() and getMemoryForReduceTask() from this method
            • No deprecated variable log messages?
          • setMaxVirtualMemoryForTask():
            • We should make sure the new variables are not exclusively set earlier before setting the old variable's value
            • No deprecated variable log messages?
          • Add a new test in TestJobConf.java to check the above two methods.
          • Logic in getMemoryForMapTask() and getMemoryForReduceTask() is a bit asymetrical. Can we fix this? In fact, the logic in the later seems incorrect. All the combinations of new and old values are:
            old value New value Value to be picked up
            Not present Anything New value
            Explicit -1 Anything New value
            value A Not present Old value
            value A Explicit -1 New value
            value A value B New value
          • Add a new test in TestJobConf.java to check the above two methods and all the above combinations.
          • getMaxVirtualMemoryForTask() and setMaxVirtualMemoryForTask(): javadoc. Mention that map and reduce memories are now specified separately in words
          • The following variable/methods need(s) to brought back, but with deprecated warnings
            • MAPRED_TASK_DEFAULT_MAXVMEM_PROPERTY
            • getMaxPhysicalMemoryForTask()
            • setMaxPhysicalMemoryForTask()
          Show
          Vinod Kumar Vavilapalli added a comment - Looked at the latest patch. Some comments: CapacityTaskScheduler.java initializeMemoryRelatedConf: New values don't take precedence with the current code Add a separate test-case for the above method in TestCapacityScheduler w.r.t deprecated values. JobTracker.java same as the above changes in CapacityTaskScheduler.java JobConf: getMaxVirtualMemoryForTask(): If new keys are present, we should return the maximum of the new values specified for map/reduce tasks Directly call getMemoryForMapTask() and getMemoryForReduceTask() from this method No deprecated variable log messages? setMaxVirtualMemoryForTask(): We should make sure the new variables are not exclusively set earlier before setting the old variable's value No deprecated variable log messages? Add a new test in TestJobConf.java to check the above two methods. Logic in getMemoryForMapTask() and getMemoryForReduceTask() is a bit asymetrical. Can we fix this? In fact, the logic in the later seems incorrect. All the combinations of new and old values are: old value New value Value to be picked up Not present Anything New value Explicit -1 Anything New value value A Not present Old value value A Explicit -1 New value value A value B New value Add a new test in TestJobConf.java to check the above two methods and all the above combinations. getMaxVirtualMemoryForTask() and setMaxVirtualMemoryForTask(): javadoc. Mention that map and reduce memories are now specified separately in words The following variable/methods need(s) to brought back, but with deprecated warnings MAPRED_TASK_DEFAULT_MAXVMEM_PROPERTY getMaxPhysicalMemoryForTask() setMaxPhysicalMemoryForTask()
          Hide
          rahul k singh added a comment -

          Have incorporated all the comments except

          1. Add a separate test-case for the above method in TestCapacityScheduler w.r.t deprecated values.

          As this is already done in TestCapacityScheduler , whenever i call the deperecated tests , in the test we are initializing the values via scheduler.start method. So initialization is tested

          Show
          rahul k singh added a comment - Have incorporated all the comments except Add a separate test-case for the above method in TestCapacityScheduler w.r.t deprecated values. As this is already done in TestCapacityScheduler , whenever i call the deperecated tests , in the test we are initializing the values via scheduler.start method. So initialization is tested
          Hide
          rahul k singh added a comment -

          removed the System.out.println statement

          Show
          rahul k singh added a comment - removed the System.out.println statement
          Hide
          Hemanth Yamijala added a comment -

          In an offline discussion, the following two comments came up:

          • The test cases seem to test every high memory test case in two modes - with the old variables and with the new variables. I think that's overkill and is making the test cases hard to maintain over a long run.
          • Some memory related variables (related to physical memory configuration) are removed in a backwards incompatible manner. If they are configured in the conf files we should print a warning message to the users. I don't know if this is already done. If yes, please ignore this.
          Show
          Hemanth Yamijala added a comment - In an offline discussion, the following two comments came up: The test cases seem to test every high memory test case in two modes - with the old variables and with the new variables. I think that's overkill and is making the test cases hard to maintain over a long run. Some memory related variables (related to physical memory configuration) are removed in a backwards incompatible manner. If they are configured in the conf files we should print a warning message to the users. I don't know if this is already done. If yes, please ignore this.
          Hide
          rahul k singh added a comment -

          Implemented the changes suggested by hemanth.

          Show
          rahul k singh added a comment - Implemented the changes suggested by hemanth.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Looked at the latest patch. Have some more comments:

          TaskTracker.java

          • in initializeMemoryManagement, need to also handle UPPER_LIMIT_ON_TASK_VMEM_PROPERTY

          CapacitySchedulerConf.java

          • This class is not public. So need for back support config values in here. Revert all the changes in this file.
          • The corresponding variables need to handled by printing out warning messages wherever they are used.

          TestCapacityScheduler.java

          • Removed spurious empty line before testDeprecatedValues

          CapacityTaskScheduler.java

          • initializeMemoryRelatedConf
            • need to handle UPPER_LIMIT_ON_TASK_VMEM_PROPERTY, limitMaxPmemForTasks, MAPRED_TASK_DEFAULT_MAXVMEM_PROPERTY, defaultPercentOfPmemInVmem
            • we are checking for presence of new map value only. It should be either map or reduce value, I think. In other words, if any of the two new config params is used, old param will be disgarded. Also modify TestCapacityScheduler.testDeprecatedValues testing this.

          JobConf.java

          • setMaxVirtualMemoryForTask - documentation is wrongly aligned.
          • getMaxVirtualMemoryForTask():
            • we are checking for presence of new map value only. It should be either map or reduce value, I think. Also modify TestJobConf testing this.
            • No deprecated variable log messages? (previous review comment missed)

          TestJobConf.java

          • Remove useless empty lines
          • Missing tests
            • getMemoryForMapTask() and getMemoryForReduceTask() : old=value A, new=Not present, result=Old value
            • tests for getMaxVirtualMemoryForTask() and setMaxVirtualMemoryForTask() (previous review comment missed)
          Show
          Vinod Kumar Vavilapalli added a comment - Looked at the latest patch. Have some more comments: TaskTracker.java in initializeMemoryManagement, need to also handle UPPER_LIMIT_ON_TASK_VMEM_PROPERTY CapacitySchedulerConf.java This class is not public. So need for back support config values in here. Revert all the changes in this file. The corresponding variables need to handled by printing out warning messages wherever they are used. TestCapacityScheduler.java Removed spurious empty line before testDeprecatedValues CapacityTaskScheduler.java initializeMemoryRelatedConf need to handle UPPER_LIMIT_ON_TASK_VMEM_PROPERTY, limitMaxPmemForTasks, MAPRED_TASK_DEFAULT_MAXVMEM_PROPERTY, defaultPercentOfPmemInVmem we are checking for presence of new map value only. It should be either map or reduce value, I think. In other words, if any of the two new config params is used, old param will be disgarded. Also modify TestCapacityScheduler.testDeprecatedValues testing this. JobConf.java setMaxVirtualMemoryForTask - documentation is wrongly aligned. getMaxVirtualMemoryForTask(): we are checking for presence of new map value only. It should be either map or reduce value, I think. Also modify TestJobConf testing this. No deprecated variable log messages? (previous review comment missed) TestJobConf.java Remove useless empty lines Missing tests getMemoryForMapTask() and getMemoryForReduceTask() : old=value A, new=Not present, result=Old value tests for getMaxVirtualMemoryForTask() and setMaxVirtualMemoryForTask() (previous review comment missed)
          Hide
          Vinod Kumar Vavilapalli added a comment -

          One more:
          JobConf.java

          • setMaxVirtualMemoryForTask : No deprecated variable log messages? (previous review comment missed)
          Show
          Vinod Kumar Vavilapalli added a comment - One more: JobConf.java setMaxVirtualMemoryForTask : No deprecated variable log messages? (previous review comment missed)
          Hide
          rahul k singh added a comment -

          Implemented changes suggested above

          Show
          rahul k singh added a comment - Implemented changes suggested above
          Hide
          rahul k singh added a comment -

          Implement some sanity issues. and changed the testcase

          Show
          rahul k singh added a comment - Implement some sanity issues. and changed the testcase
          Hide
          rahul k singh added a comment -

          The new patch gives higher preference to old config settings. It takes precedence over the new settings.

          Show
          rahul k singh added a comment - The new patch gives higher preference to old config settings. It takes precedence over the new settings.
          Hide
          rahul k singh added a comment -

          Handled some corner cases.

          Show
          rahul k singh added a comment - Handled some corner cases.
          Hide
          rahul k singh added a comment -

          addressed failed test scenario for TestCapacityScheduler. and also made sure that getMaxVirtualMemoryForTask is proper

          Show
          rahul k singh added a comment - addressed failed test scenario for TestCapacityScheduler. and also made sure that getMaxVirtualMemoryForTask is proper
          Hide
          Vinod Kumar Vavilapalli added a comment -

          The latest changes look good to me. +1 for the patch.

          Show
          Vinod Kumar Vavilapalli added a comment - The latest changes look good to me. +1 for the patch.
          Hide
          rahul k singh 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 6 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 does not introduce any new Findbugs warnings.
          [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 - [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 6 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 does not introduce any new Findbugs warnings. [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
          rahul k singh added a comment -

          attached patch for 20 branch.

          Show
          rahul k singh added a comment - attached patch for 20 branch.
          Hide
          rahul k singh added a comment -

          Uploading the new patch to rectify the javadoc warning.

          [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 6 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 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 - Uploading the new patch to rectify the javadoc warning. [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 6 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 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
          rahul k singh added a comment -

          ran ant test .

          TestKillSubProcesses failed . there is already jira MAPREDUCE - 408 to handle that issue.

          rest everything passed.

          Show
          rahul k singh added a comment - ran ant test . TestKillSubProcesses failed . there is already jira MAPREDUCE - 408 to handle that issue. rest everything passed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12413299/hadoop-5919-13-20.patch
          against trunk revision 794101.

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

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

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

          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/392/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/12413299/hadoop-5919-13-20.patch against trunk revision 794101. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/392/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          The -1 is most likely because Hudson ran the tests on the last patch file which was for the 20 branch and not trunk. Rahul has already uploaded results for test-patch and test on the trunk patch. So, I am ready to commit this.

          Show
          Hemanth Yamijala added a comment - The -1 is most likely because Hudson ran the tests on the last patch file which was for the 20 branch and not trunk. Rahul has already uploaded results for test-patch and test on the trunk patch. So, I am ready to commit this.
          Hide
          Hemanth Yamijala added a comment -

          I found a couple of places where warning messages are not being printed while using the old values. Examples are JobTracker.initializeTaskMemoryRelatedConfig, CapacityTaskScheduler.initializeMemoryRelatedConf, the deprecated JobConf APIs and the new APIs when we return the old values. Can you please change this and upload a new patch ?

          Show
          Hemanth Yamijala added a comment - I found a couple of places where warning messages are not being printed while using the old values. Examples are JobTracker.initializeTaskMemoryRelatedConfig, CapacityTaskScheduler.initializeMemoryRelatedConf, the deprecated JobConf APIs and the new APIs when we return the old values. Can you please change this and upload a new patch ?
          Hide
          Chris Douglas added a comment -

          Canceling while Hemanth's comments are addressed

          Show
          Chris Douglas added a comment - Canceling while Hemanth's comments are addressed
          Hide
          rahul k singh added a comment -

          Attached the fix with hemanth's comments

          ran ant test it ran fine

          [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 6 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 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 - Attached the fix with hemanth's comments ran ant test it ran fine [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 6 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 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 -

          Looks fine. One minor nit would be to print the deprecated messages in the JobConf API in terms of the new APIs to use, instead of the new variables to use. Other than that +1.

          Show
          Hemanth Yamijala added a comment - Looks fine. One minor nit would be to print the deprecated messages in the JobConf API in terms of the new APIs to use, instead of the new variables to use. Other than that +1.
          Hide
          rahul k singh added a comment -

          ran test -patch

          [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 6 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 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]

          BUILD SUCCESSFUL

          Show
          rahul k singh added a comment - ran test -patch [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 6 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 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] BUILD SUCCESSFUL
          Hide
          Hemanth Yamijala added a comment -

          I just committed this to trunk and branch 0.20. Thanks, Rahul !

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

          Integrated in Hadoop-Mapreduce-trunk #38 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/38/)
          8. Fixes an assertion problem in TestKillSubProcesses. Contributed by Ravi Gummadi.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #38 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/38/ ) 8. Fixes an assertion problem in TestKillSubProcesses. Contributed by Ravi Gummadi.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development