[
https://issues.apache.org/jira/browse/HADOOP-5919?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12718378#action_12718378
]
Vinod K V commented on HADOOP-5919:
-----------------------------------
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:
{code}
if (mapping.getKeyMappings() != null) {
for (String newKey : mapping.getKeyMappings()) {
properties.setProperty(newKey, value);
}
}
LOG.warn(mapping.getMessage());
return true;
{code}
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.
> Memory management variables need a backwards compatibility option after
> HADOOP-5881
> -----------------------------------------------------------------------------------
>
> Key: HADOOP-5919
> URL: https://issues.apache.org/jira/browse/HADOOP-5919
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Reporter: Hemanth Yamijala
> Assignee: rahul k singh
> Priority: Blocker
> Attachments: hadoop-5919-1.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
