YARN-11250. Capture the Performance Metrics of ZookeeperFederationStateStore. #4738
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
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help review this pr? I think some metrics should be added to ZookeeperFederationStateStore. Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 15, 2022
| @@ -54,25 +54,25 @@ public final class ZKFederationStateStoreOpDurations implements MetricsSource { | |||
| private MutableRate registerSubClusterCall; | |||
|
|
|||
| @Metric("Duration for a deregister subCluster call") | |||
| private MutableRate deregisterSubCluster; | |||
| private MutableRate deregisterSubClusterCall; | |||
There was a problem hiding this comment.
Do we need to add the Call at the end?
There was a problem hiding this comment.
Thanks for your suggestion, there is no need to add the word Call after the variable, I will modify the code.
goiri
approved these changes
Aug 15, 2022
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 16, 2022
| @@ -104,29 +107,39 @@ | |||
| public class ZookeeperFederationStateStore implements FederationStateStore { | |||
|
|
|||
| private static final Logger LOG = | |||
| LoggerFactory.getLogger(ZookeeperFederationStateStore.class); | |||
| LoggerFactory.getLogger(ZookeeperFederationStateStore.class); | |||
There was a problem hiding this comment.
Thanks for your help reviewing the code, I will fix it.
|
|
||
| private final static String ROOT_ZNODE_NAME_MEMBERSHIP = "memberships"; | ||
| private final static String ROOT_ZNODE_NAME_APPLICATION = "applications"; | ||
| private final static String ROOT_ZNODE_NAME_POLICY = "policies"; | ||
|
|
||
| /** Interface to Zookeeper. */ | ||
| /** | ||
| * Interface to Zookeeper. |
There was a problem hiding this comment.
The old format is common; I would leave it as is
| @Override | ||
| public void init(Configuration conf) throws YarnException { | ||
| LOG.info("Initializing ZooKeeper connection"); | ||
|
|
||
| baseZNode = conf.get( | ||
| YarnConfiguration.FEDERATION_STATESTORE_ZK_PARENT_PATH, | ||
| YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_ZK_PARENT_PATH); | ||
| YarnConfiguration.FEDERATION_STATESTORE_ZK_PARENT_PATH, |
| @@ -203,53 +217,58 @@ public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster( | |||
| FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg); | |||
| } | |||
| SubClusterId newSubClusterId = | |||
| request.getApplicationHomeSubCluster().getHomeSubCluster(); | |||
| request.getApplicationHomeSubCluster().getHomeSubCluster(); | |||
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 17, 2022
| @@ -161,8 +169,9 @@ public void close() throws Exception { | |||
|
|
|||
| @Override | |||
| public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster( | |||
| AddApplicationHomeSubClusterRequest request) throws YarnException { | |||
| AddApplicationHomeSubClusterRequest request) throws YarnException { | |||
There was a problem hiding this comment.
I will fix the indentation problem.
| @@ -184,16 +193,17 @@ public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster( | |||
| FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg); | |||
| } | |||
|
|
|||
| return AddApplicationHomeSubClusterResponse | |||
| .newInstance(homeSubCluster); | |||
| opDurations.addAppHomeSubClusterDuration(clock.getTime() - start); | |||
There was a problem hiding this comment.
No need to do this but one thing I was thinking about is the fact that we keep doing the substraction in the call.
Would it look better to pass the start and the end time and do the substraction inside?
.../java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java
Show resolved
Hide resolved
| return GetApplicationHomeSubClusterResponse.newInstance( | ||
| ApplicationHomeSubCluster.newInstance(appId, homeSubCluster)); | ||
| ApplicationHomeSubCluster.newInstance(appId, homeSubCluster)); |
| } | ||
|
|
||
| @Override | ||
| public GetApplicationsHomeSubClusterResponse getApplicationsHomeSubCluster( | ||
| GetApplicationsHomeSubClusterRequest request) throws YarnException { | ||
| GetApplicationsHomeSubClusterRequest request) throws YarnException { |
| FederationApplicationHomeSubClusterStoreInputValidator.validate(request); | ||
| ApplicationId appId = request.getApplicationId(); | ||
| SubClusterId homeSubCluster = getApp(appId); | ||
| if (homeSubCluster == null) { | ||
| String errMsg = "Application " + appId + " does not exist"; | ||
| FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg); | ||
| } | ||
| opDurations.addGetAppHomeSubClusterDuration(clock.getTime() - start); | ||
| return GetApplicationHomeSubClusterResponse.newInstance( |
There was a problem hiding this comment.
As this is all we do, would it make sense to have a:
GetApplicationHomeSubClusterResponse.newInstance(appId, homeSubCluster);
?
There was a problem hiding this comment.
Your suggestion is reasonable, I will modify the code.
| return SubClusterHeartbeatResponse.newInstance(); | ||
| } | ||
|
|
||
| @Override | ||
| public GetSubClusterInfoResponse getSubCluster( | ||
| GetSubClusterInfoRequest request) throws YarnException { | ||
|
|
||
| GetSubClusterInfoRequest request) throws YarnException { |
|
@goiri Please help to review the code again, thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Aug 17, 2022
| registerSubCluster.add(endTime - startTime); | ||
| } | ||
|
|
||
| public void addDeregisterSubClusterDuration(long endTime, long startTime) { |
There was a problem hiding this comment.
Can we do long startTime, long endTime?
It looks a little more intuitive.
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
goiri
approved these changes
Aug 17, 2022
|
🎊 +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. |
|
💔 -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 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-11250. Capture the Performance Metrics of ZookeeperFederationStateStore.