[
https://issues.apache.org/jira/browse/MAPREDUCE-476?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12738811#action_12738811
]
Vinod K V commented on MAPREDUCE-476:
-------------------------------------
Replies in-line.
bq. Is there anything blocking MAPREDUCE-711 that prevents it from being
committed?
No, I've asked Owen to commit it.
bq. I had a very clever bug in there(caused by not thinking enough while
resolving a merge conflict) that deleted the current working directory,
recursively, in one of the tests. (TaskRunner is hard-coded to delete current
working directory, which is ok, since it's typically a child process; not ok
for LocalJobRunner.)
Nice catch! I too had burnt myself once trying to refactor out the code in
Child.java to a thread, and ended up deleting my whole work space because of
the same issue :(
bq. The tricky bit is always fixing only the lines I've changed, and not all
the lines in a given file, to preserve history and keep reviewing sane.
That is correct.
bq. 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.
bq. Don't think I agree here. We can deprecate the getLocalCache methods in
DistributedCache right away. They delegate to each other, and one of them
delegates to TrackerDistributedCacheManager. Ideally, I'd remove these
altogether — Hadoop internally does not use these methods with this patch, and
there's no sensible reason why someone else would, but since it's public, it's
getting deprecated. But it's not being deprecated with a pointer to use
something else; it's getting deprecated so that you don't use it at all.
Okay, i thought those methods were used internally atleast. But if, as you've
said, they are not used at all internally, +1 for deprecating them all together.
bq. Using .equals method at +150 if (cacheFile.type ==
CacheFile.FileType.ARCHIVE). If you feel strongly about this, happy to change
it, but I think == is more consistent.
I am fine, no problem with this.
bq. TaskTracker.initialize() A new DistributedCacheManager is created every
time, so old files will not be deleted by the subsequent purgeCache. Either we
should have only one cacheManager for a TaskTracker or
DistributedCacheManager.cachedArchives should be static. The same problem
exists with the deprecated purgeCache() method in DistributedCache.java
bq. If my understanding is correct, in the course of normal operations,
TaskTracker.initialize() is only called once, by TaskTracker.run(). At startup
time (and whenever the TaskTracker decides to lose all of its state and reset
itself, which is essentially the same), the TaskTracker initializes the
TrackerDistributedCacheManager object. This is also the only time it's allowed
to "clear" the cache, since there are for certain no tasks running at
initialization time that depend on those.
Ah, I was under the impression that distribute cache files are purged
EXPLICITLY when a TT reinitializes, but it looks like we do a full delete on
TaskTracker's local files during re-init. So, the code in your patch will work
for now, but it won't be in future when we might need explicit purge of
distributed cache files when they are owned by users themselves (HADOOP-4493).
We will change it then, +1 for the current changes.
bq. JobClient.java What happens to the code in here? Should this be refactored
too/ are we going to do something about it?
bq. Good question I do think that JobClient should be using some methods in the
filecache package, instead of hard-coding a lot of logic. That said, I chose to
stop somewhere to avoid making this patch even harder to review. I think it's
ripe for a future JIRA.
Okay.
bq. I think most of the getter/setter methods are deprecable and moveable to
DistributedCacheManager. Or at least we should give some thought about it. For
most of them, I do see from the javadoc that they are only for internal use
anyways.
bq. I agree. A few of them are used to manage the Configuration object. (In my
mind, we're serializing and de-serializing a set of requirements for the
distributed cache into the text configuration, and doing so a bit haphazardly.)
I was very tempted to remove all the ones that are only meant to be internal,
but Tom advised me that I need to keep them deprecated for a version. Again, I
think moving those methods into a more private place is a good task to do along
with changing how JobClient calls into this stuff.
+1. So are you planning to do in the next version or in this patch itself?
bq. I don't use Pipes typically, and it doesn't seem to compile on my Mac. I'll
try it on a Linux machine, but if it's easy/handy for you, it'd be great to
verify that bug.
Will do so.
bq. TestDistributedCacheManager * Code related to setFileStamps in
JobClient.java (+636 to +656) and testManagerFlow() (+71 to +74) can be
refactored into an internal-only method in DistributedCacheManager.
bq. I extracted the method and moved it to TrackerDistributedCacheManager. Now
that that has "Tracker" in the name, it's a little misplaced, but it's not too
bad.
I think that's agreeable for now. We can move it to a more appropriate place in
future if we find one.
bq. Another point. We should consolidate TestMRWithDistributedCache,
TestMiniMRLocalFS and TestMiniMRDFSCaching. That's a lot of code in three
different files testing mostly common code. May be another JIRA issue if you
feel so.
bq. I'd prefer a separate JIRA.
Fine. +1.
> 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-vs-v3.patch, MAPREDUCE-476-v2-vs-v3.try2.patch,
> MAPREDUCE-476-v2-vs-v4.txt, MAPREDUCE-476-v2.patch, MAPREDUCE-476-v3.patch,
> MAPREDUCE-476-v3.try2.patch, MAPREDUCE-476-v4-requires-MR711.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.
