[ 
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)

Reply via email to