YARN-10885. Make FederationStateStoreFacade#getApplicationHomeSubCluster use JCache. #4701
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 4, 2022 23:05
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Aug 4, 2022
| @@ -169,7 +169,7 @@ public void testGetPoliciesConfigurations() throws YarnException { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testGetHomeSubClusterForApp() throws YarnException { | |||
| public void getApplicationHomeSubCluster() throws YarnException { | |||
There was a problem hiding this comment.
I think we use test for the method name
| ApplicationId applicationId = ApplicationId.fromString(appId); | ||
| GetApplicationHomeSubClusterResponse response = | ||
| stateStore.getApplicationHomeSubCluster( | ||
| GetApplicationHomeSubClusterRequest.newInstance(applicationId)); |
There was a problem hiding this comment.
Let's create a method that takes string and does the appId transformation inside.
There was a problem hiding this comment.
Thanks for your suggestion, I will fix it.
| if (isCachingEnabled()) { | ||
| Map<ApplicationId, SubClusterId> cacheAppSubCluster = | ||
| (Map<ApplicationId, SubClusterId>) | ||
| cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString())); |
There was a problem hiding this comment.
Extract things a little.
buildGetApplicationHomeSubClusterRequest could also take ApplicationId.
goiri
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, thank you very much! |
goiri
reviewed
Aug 4, 2022
| if (isCachingEnabled()) { | ||
| Map<ApplicationId, SubClusterId> cacheAppSubCluster = | ||
| (Map<ApplicationId, SubClusterId>) | ||
| cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString())); |
| @@ -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.
Thanks for your suggestion, I will modify the code.
| response.getApplicationHomeSubCluster(); | ||
| SubClusterId subClusterId = applicationHomeSubCluster.getHomeSubCluster(); | ||
|
|
||
| Map<String, SubClusterId> appSubCluster = new HashMap<>(); |
| facade.addApplicationHomeSubCluster(appHomeSubCluster); | ||
|
|
||
| SubClusterId subClusterIdCache = facade.getApplicationHomeSubCluster(appId); | ||
| Assert.assertEquals(subClusterIdCache, subClusterIdAdd); |
There was a problem hiding this comment.
How do we make sure we went through the cache?
There was a problem hiding this comment.
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.
@RunWith(Parameterized.class)
public class TestFederationStateStoreFacade {
public TestFederationStateStoreFacade(Boolean isCachingEnabled) {
conf = new Configuration();
if (!(isCachingEnabled.booleanValue())) {
conf.setInt(YarnConfiguration.FEDERATION_CACHE_TIME_TO_LIVE_SECS, 0);
}
}
}
There was a problem hiding this comment.
I refactored the test for getting objects from Cache. If the test case enables Cache mode, the comparison will be as follows:
Whether the object obtained directly from the Cache and the object obtained from the Facade are the same.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, thank you very much! |
goiri
reviewed
Aug 6, 2022
| if (isCachingEnabled()) { | ||
| Object value = | ||
| cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString())); | ||
| if(value instanceof SubClusterId){ |
There was a problem hiding this comment.
Thanks for your suggestion, I will fix it.
| try { | ||
| if (isCachingEnabled()) { | ||
| Object value = | ||
| cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString())); |
There was a problem hiding this comment.
Add ApplicationId as the argument and tostring inside.
There was a problem hiding this comment.
I will modify the code.
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, Thank you very much! |
goiri
reviewed
Aug 8, 2022
| stateStore.getApplicationHomeSubCluster( | ||
| try { | ||
| if (isCachingEnabled()) { | ||
| Object value = |
There was a problem hiding this comment.
Thanks for your help reviewing the code, I will fix it.
| if (value instanceof SubClusterId) { | ||
| return (SubClusterId) value; | ||
| } else { | ||
| throw new YarnException("Cannot be converted to SubClusterId."); |
There was a problem hiding this comment.
Maybe add the type of the original.
There was a problem hiding this comment.
This part of the code does need cast, I simplified the code.
The main reasons are as follows:
The key and value used by the Cache defined here are both Object types.
private Cache<Object, Object> cache;
| input -> { | ||
| GetApplicationHomeSubClusterResponse response = | ||
| stateStore.getApplicationHomeSubCluster( | ||
| GetApplicationHomeSubClusterRequest.newInstance(applicationId)); |
There was a problem hiding this comment.
Extract the request and probably do indentation like:
CacheRequest<String, SubClusterId> cacheRequest = new CacheRequest<>(
cacheKey,
|
|
||
| ApplicationHomeSubCluster appHomeSubCluster = | ||
| ApplicationHomeSubCluster.newInstance(appId, subClusterId1); | ||
| SubClusterId subClusterIdAdd = |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 9, 2022
| input -> { | ||
| GetApplicationHomeSubClusterResponse response = | ||
| stateStore.getApplicationHomeSubCluster( | ||
| GetApplicationHomeSubClusterRequest.newInstance(applicationId)); |
There was a problem hiding this comment.
Thanks for your suggestion, I will fix it.
| stateStore.getApplicationHomeSubCluster( | ||
| GetApplicationHomeSubClusterRequest.newInstance(applicationId)); | ||
|
|
||
| ApplicationHomeSubCluster applicationHomeSubCluster = |
|
@goiri Please help to review the code again, thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Aug 10, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Thank you very much for helping to review the code! |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: YARN-10885. add FederationStateStoreFacade#getApplicationHomeSubCluster use JCache.
JunitTest will be added later.