hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Karthik Kambatla (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2180) In-memory backing store for cache manager
Date Sat, 27 Sep 2014 01:41:34 GMT

    [ https://issues.apache.org/jira/browse/YARN-2180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14150324#comment-14150324
] 

Karthik Kambatla commented on YARN-2180:
----------------------------------------

Thanks for the updates, Chris. The overall approach looks good. Review comments:
# SharedCacheManager#createSCMStoreService should use ReflectionUtils.newInstance. RMProxy
is an example. 
# Thinking out loud here. YarnConfiguration (and yarn-default): I was wondering if we need
a separate prefix for "manager". Do we have more configs coming later specific to manager?
yarn.sharedcache.store is not ambiguous.
# SCMStore
## A couple of lines longer than 80 chars. 
## For resources that are not in the store, isn't the access time trivially zero? I am okay
with returning -1 for those cases, but will returning zero help at call sites? 
## Nit: Would keep the methods concerning references all together.
# InMemorySCMStore configuration - do we need a separate configuration class for in-memory-store?
Why not include it in YarnConfiguration similar to RMStore implementations? According to what
we decide on, we might want to change the actual config names. 
# InMemorySCMStore
## Can we rename "map" to something more descriptive? cacheResources? 
## Nit: Move bootstrapping code to a different method for readability? 
## Isn't the following synchronized block prone to races when different threads lock on different
objects? 
{code}
    synchronized (initialApps) {
      initialApps = getInitialApps(conf);
    }
{code}
## We can leave it as is for now, but the implementation of AppChecker should come from some
util method based on whether it is embedded or not. If we are open to it, we can add that
method now an make it return RemoteAppChecker by default. 
## Nit: The following should fit on two lines:
{code}
  Map<String, String> getInitialCachedResources(FileSystem fs,
      Configuration conf)
      throws IOException {
{code}
## Use containsKey instead of the following? 
{code}
            String mapped = initialCachedEntries.get(key);
            if (mapped != null) {
{code}
## clearCache() - we should annotate each TODO with a follow-up JIRA, so we don't forget.

> In-memory backing store for cache manager
> -----------------------------------------
>
>                 Key: YARN-2180
>                 URL: https://issues.apache.org/jira/browse/YARN-2180
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Chris Trezzo
>            Assignee: Chris Trezzo
>         Attachments: YARN-2180-trunk-v1.patch, YARN-2180-trunk-v2.patch, YARN-2180-trunk-v3.patch,
YARN-2180-trunk-v4.patch, YARN-2180-trunk-v5.patch
>
>
> Implement an in-memory backing store for the cache manager.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message