-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
YARN-10885. Make FederationStateStoreFacade#getApplicationHomeSubCluster use JCache. #4701
Changes from 4 commits
d45e599
6ad9607
1454a06
0c9d648
12d77ad
0eefb19
f6096e2
ce5ca51
fb42275
7a9c9a5
0ed4dd4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,8 @@ public final class FederationStateStoreFacade { | |
private static final String GET_SUBCLUSTERS_CACHEID = "getSubClusters"; | ||
private static final String GET_POLICIES_CONFIGURATIONS_CACHEID = | ||
"getPoliciesConfigurations"; | ||
private static final String GET_APPLICATION_HOME_SUBCLUSTER_CACHEID = | ||
"getApplicationHomeSubCluster"; | ||
|
||
private static final FederationStateStoreFacade FACADE = | ||
new FederationStateStoreFacade(); | ||
|
@@ -376,10 +378,20 @@ public void updateApplicationHomeSubCluster( | |
*/ | ||
public SubClusterId getApplicationHomeSubCluster(ApplicationId appId) | ||
throws YarnException { | ||
GetApplicationHomeSubClusterResponse response = | ||
stateStore.getApplicationHomeSubCluster( | ||
try { | ||
if (isCachingEnabled()) { | ||
Map<String, SubClusterId> cacheAppSubCluster = | ||
(Map<String, SubClusterId>) | ||
cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString())); | ||
return cacheAppSubCluster.get(appId); | ||
} else { | ||
GetApplicationHomeSubClusterResponse response = stateStore.getApplicationHomeSubCluster( | ||
GetApplicationHomeSubClusterRequest.newInstance(appId)); | ||
return response.getApplicationHomeSubCluster().getHomeSubCluster(); | ||
return response.getApplicationHomeSubCluster().getHomeSubCluster(); | ||
} | ||
} catch (Throwable ex) { | ||
throw new YarnException(ex); | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -513,6 +525,32 @@ public Map<String, SubClusterPolicyConfiguration> invoke( | |
return cacheRequest; | ||
} | ||
|
||
@SuppressWarnings("unchecked") | ||
private Object buildGetApplicationHomeSubClusterRequest(String appId) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we cast it here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your suggestion, I will modify the code. |
||
final String cacheKey = buildCacheKey(getClass().getSimpleName(), | ||
GET_APPLICATION_HOME_SUBCLUSTER_CACHEID, appId); | ||
CacheRequest<String, Map<String, SubClusterId>> cacheRequest = | ||
new CacheRequest<>( | ||
cacheKey, | ||
input -> { | ||
ApplicationId applicationId = ApplicationId.fromString(appId); | ||
|
||
GetApplicationHomeSubClusterResponse response = | ||
stateStore.getApplicationHomeSubCluster( | ||
GetApplicationHomeSubClusterRequest.newInstance(applicationId)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extract the request and probably do indentation like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will fix it. |
||
|
||
ApplicationHomeSubCluster applicationHomeSubCluster = | ||
response.getApplicationHomeSubCluster(); | ||
SubClusterId subClusterId = applicationHomeSubCluster.getHomeSubCluster(); | ||
|
||
Map<String, SubClusterId> appSubCluster = new HashMap<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you return a singleton map? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will fix it. |
||
appSubCluster.put(appId, subClusterId); | ||
|
||
return appSubCluster; | ||
}); | ||
return cacheRequest; | ||
} | ||
|
||
protected String buildCacheKey(String typeName, String methodName, | ||
String argName) { | ||
StringBuilder buffer = new StringBuilder(); | ||
|
@@ -560,7 +598,7 @@ private static class CacheRequest<K, V> { | |
private K key; | ||
private Func<K, V> func; | ||
|
||
public CacheRequest(K key, Func<K, V> func) { | ||
CacheRequest(K key, Func<K, V> func) { | ||
this.key = key; | ||
this.func = func; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,4 +206,19 @@ public void testAddApplicationHomeSubCluster() throws YarnException { | |
Assert.assertEquals(subClusterId1, result); | ||
} | ||
|
||
@Test | ||
public void testGetApplicationHomeSubClusterCache() throws YarnException { | ||
ApplicationId appId = ApplicationId.newInstance(clusterTs, numApps + 1); | ||
SubClusterId subClusterId1 = SubClusterId.newInstance("Home1"); | ||
|
||
ApplicationHomeSubCluster appHomeSubCluster = | ||
ApplicationHomeSubCluster.newInstance(appId, subClusterId1); | ||
SubClusterId subClusterIdAdd = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will fix it. |
||
facade.addApplicationHomeSubCluster(appHomeSubCluster); | ||
|
||
SubClusterId subClusterIdCache = facade.getApplicationHomeSubCluster(appId); | ||
Assert.assertEquals(subClusterIdCache, subClusterIdAdd); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we make sure we went through the cache? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to this test class. This test class uses junit's Parameterized, which will be tested twice. The cache effective configuration is initialized in the constructor. The first setting cache does not take effect, and the second setting cache takes effect.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored the test for getting objects from Cache. If the test case enables Cache mode, the comparison will be as follows: |
||
Assert.assertEquals(subClusterId1, subClusterIdAdd); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract things a little.
buildGetApplicationHomeSubClusterRequest could also take ApplicationId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still left.