Skip to content

YARN-10885. Make FederationStateStoreFacade#getApplicationHomeSubCluster use JCache. #4701

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.

Sign up for GitHub

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

Merged
goiri merged 11 commits into from
Aug 15, 2022
Merged

YARN-10885. Make FederationStateStoreFacade#getApplicationHomeSubCluster use JCache. #4701

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d45e599
YARN-10885. add FederationStateStoreFacade#getApplicationHomeSubClust…
Aug 4, 2022
6ad9607
YARN-10885. Add Junit Test.
Aug 4, 2022
1454a06
YARN-10885. Fix CheckStyle.
Aug 4, 2022
0c9d648
YARN-10885. Fix CheckStyle.
Aug 4, 2022
12d77ad
YARN-10885. add FederationStateStoreFacade#getApplicationHomeSubClust…
Aug 5, 2022
0eefb19
YARN-10885. Fix CheckStyle.
Aug 5, 2022
f6096e2
YARN-10885. Modify Some Junit Test.
Aug 5, 2022
ce5ca51
YARN-10885. Fix CheckStyle.
Aug 7, 2022
fb42275
YARN-10885. Fix CheckStyle.
Aug 9, 2022
7a9c9a5
YARN-10885. Fix CheckStyle.
Aug 10, 2022
0ed4dd4
Merge branch 'trunk' into YARN-10885
slfan1989 Aug 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions .../main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -376,10 +378,20 @@ public void updateApplicationHomeSubCluster(
*/
public SubClusterId getApplicationHomeSubCluster(ApplicationId appId)
throws YarnException {
GetApplicationHomeSubClusterResponse response =
stateStore.getApplicationHomeSubCluster(
try {
if (isCachingEnabled()) {
Map<ApplicationId, SubClusterId> cacheAppSubCluster =
(Map<ApplicationId, SubClusterId>)
cache.get(buildGetApplicationHomeSubClusterRequest(appId.toString()));
Copy link
Member

@goiri goiri Aug 4, 2022

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.

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix it.

Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still left.

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);
}
}

/**
Expand Down Expand Up @@ -513,6 +525,31 @@ public Map<String, SubClusterPolicyConfiguration> invoke(
return cacheRequest;
}

private Map<ApplicationId, SubClusterId> buildApplicationSubClusterMap(
GetApplicationHomeSubClusterResponse response) {
ApplicationHomeSubCluster applicationHomeSubCluster = response.getApplicationHomeSubCluster();
SubClusterId subClusterId = applicationHomeSubCluster.getHomeSubCluster();
ApplicationId applicationId = applicationHomeSubCluster.getApplicationId();
Map<ApplicationId, SubClusterId> appSubCluster = new HashMap<>();
appSubCluster.put(applicationId, subClusterId);
return appSubCluster;
}

private Object buildGetApplicationHomeSubClusterRequest(String appId) {
Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we cast it here?

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 5, 2022

Choose a reason for hiding this comment

The 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<ApplicationId, SubClusterId>> cacheRequest =
new CacheRequest<>(cacheKey,
input -> {
ApplicationId applicationId = ApplicationId.fromString(appId);
GetApplicationHomeSubClusterResponse response =
stateStore.getApplicationHomeSubCluster(
GetApplicationHomeSubClusterRequest.newInstance(applicationId));
Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is not correct.

Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a method that takes string and does the appId transformation inside.

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion, I will fix it.

return buildApplicationSubClusterMap(response);
});
return cacheRequest;
}

protected String buildCacheKey(String typeName, String methodName,
String argName) {
StringBuilder buffer = new StringBuilder();
Expand Down
17 changes: 16 additions & 1 deletion ...t/java/org/apache/hadoop/yarn/server/federation/utils/TestFederationStateStoreFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public void testGetPoliciesConfigurations() throws YarnException {
}

@Test
public void testGetHomeSubClusterForApp() throws YarnException {
public void getApplicationHomeSubCluster() throws YarnException {
Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we use test for the method name

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix it.

for (int i = 0; i < numApps; i++) {
ApplicationId appId = ApplicationId.newInstance(clusterTs, i);
Assert.assertEquals(stateStoreTestUtil.queryApplicationHomeSC(appId),
Expand Down Expand Up @@ -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 =
Copy link
Member

@goiri goiri Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One line?

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 9, 2022

Choose a reason for hiding this comment

The 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);
Copy link
Member

@goiri goiri Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we make sure we went through the cache?

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 5, 2022

Choose a reason for hiding this comment

The 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.

@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);
    }
  }
}

Copy link
Contributor Author

@slfan1989 slfan1989 Aug 5, 2022

Choose a reason for hiding this comment

The 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:
Whether the object obtained directly from the Cache and the object obtained from the Facade are the same.

Assert.assertEquals(subClusterId1, subClusterIdAdd);
}

}