[
https://issues.apache.org/jira/browse/MAPREDUCE-476?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731899#action_12731899
]
Vinod K V commented on MAPREDUCE-476:
-------------------------------------
I've started looking at your latest patch in combination with an intermediate
patch for MAPREDUCE-711. There is quite a bit of refactoring in this patch,
though I find it really useful. Just to be sure, I will call this out
explicitly to Devaraj/Owen offline.
I have some comments in relation to the patch.
General:
- Please make sure that in the newly added code, lines aren't longer than 80
characters. For e.g, see DistributedCacheManager.newTaskHandle() method.
- Just a thought, can the classes be better renamed to reflect their usage,
something like TrackerDistributedCacheManager and TaskDistributeCacheManager?
DistributedCacheManager
- Explicitly state in javadoc that it is not a public interface :)
- This class should also have the variable number argument getLocalCache()
methods so that the corresponding methods in DistributedCache can be
deprecated. Also, each method in DistributedCache should call the correponding
method in DistributedCacheManager class.
DistributedCacheHandle
- Explicitly state in javadoc that it is not a public interface
- CacheFile.makeCacheFiles()
-- isClassPath can be renamed to shouldBePutInClasspath
-- paths can be renamed to pathsToPutInClasspath.
-- Use .equals method at +150 {code}if (cacheFile.type ==
CacheFile.FileType.ARCHIVE){code}
-- {code}static void makeCacheFiles(URI[] uris, String[] timestamps,
Path[] paths,
boolean isArchive, List<CacheFile> target){code} should instead be
{code}static void makeCacheFiles(URI[] uris, String[] timestamps,
Path[] paths,
FileType fileType, List<CacheFile> target){code}
-- I think it would be cleaner to return target instead of passing it as
an argument.
-- makeCacheFiles() method should be documented
- setup()
-- This method is really useful, avoids a lot of code duplication!
-- Leave localTaskFile writing business back in TaskRunner itself. I think
It is the task's responsiblity, not the DistributeCacheHandle's
-- cacheSubdir can better be an argument to setup() method instead of
passing it to the constructor.
- getClassPaths() : Document that it has to be called and useful only when is
already invoked.
> extend DistributedCache to work locally (LocalJobRunner)
> --------------------------------------------------------
>
> Key: MAPREDUCE-476
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-476
> Project: Hadoop Map/Reduce
> Issue Type: Improvement
> Reporter: sam rash
> Assignee: Philip Zeyliger
> Priority: Minor
> Attachments: HADOOP-2914-v1-full.patch,
> HADOOP-2914-v1-since-4041.patch, HADOOP-2914-v2.patch, HADOOP-2914-v3.patch,
> MAPREDUCE-476-v2.patch, MAPREDUCE-476.patch
>
>
> The DistributedCache does not work locally when using the outlined recipe at
> http://hadoop.apache.org/core/docs/r0.16.0/api/org/apache/hadoop/filecache/DistributedCache.html
>
> Ideally, LocalJobRunner would take care of populating the JobConf and copying
> remote files to the local file sytem (http, assume hdfs = default fs = local
> fs when doing local development.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
