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