You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "zpinto (via GitHub)" <gi...@apache.org> on 2023/10/12 09:34:55 UTC
[PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
zpinto opened a new pull request, #2657:
URL: https://github.com/apache/helix/pull/2657
Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time. This will lead to major performance gains as this was previously called in several parts of BestPossibleStateCalculation and CurrentStateComputation.
### Description
During profiling we realized that the majority(up to 90%) of the CurrentStateComputationStage is taken up by `_objectMapper.readValue`.
After investigating further, this problem is not only seen in CurrentStateComputationStage. By only deserializing on the first call to `getPartitionCapacityMap` we can **tremendously** improve the pipeline performance in more than one place.
Where is this called:
- CurrentStateComputationStage
- For every resource when reporting resource capacity metrics
- For every partition of WAGED resource when checkAndReduceInstanceCapacity based of currentStateMap
- Every pending STATE_TRANSITION message for WAGED replica to checkAndReduceInstanceCapacity (n + 1 respect capacity solution)
- BestPossibleStateCalcStage
- For checkAndReduceCapacity of every instanceToAdd for all partitions of WAGED resources (n + 1 respect capacity solution)
- Called for the creation of every single AssignableReplica for all replicas of WAGED resources
Basically, every time(unbounded as cluster grows) getPartitionCapacityWeightMap is called it is deserializing the PARTITION_CAPACITY_MAP mapField in the ResourceConfig over and over again. This change makes sure that deserialize only happens once per resource and will save lots of time.
### Tests
No behavior changes, so will rely on current tests and CI to validate.
### Changes that Break Backward Compatibility (Optional)
NA
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2657:
URL: https://github.com/apache/helix/pull/2657#issuecomment-1769071631
This PR is ready to be merged.
Final commit message:
Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time. This will lead to major performance gains as this was previously called in several parts of BestPossibleStateCalculation and CurrentStateComputation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2657:
URL: https://github.com/apache/helix/pull/2657#discussion_r1357268766
##########
helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java:
##########
@@ -389,6 +390,12 @@ public void setPreferenceLists(Map<String, List<String>> instanceLists) {
* @throws IOException - when JSON conversion fails
*/
public Map<String, Map<String, Integer>> getPartitionCapacityMap() throws IOException {
+ // It is very expensive to deserialize the partition capacity map every time this is called.
+ // Cache the deserialized map to avoid the overhead.
+ if (_deserializedPartitionCapacityMap != null) {
Review Comment:
We do not call `setPartitionCapacityMap` anywhere but tests. Cache is refreshed by replacing ResourceConfig HelixProperty in dataProvider when there is a keyChange. Replacing the property will ensure that _deserializedPartitionCapacityMap will be recomputed if ResourceConfig partition capacity maps are changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on PR #2657:
URL: https://github.com/apache/helix/pull/2657#issuecomment-1759642090
First and foremost this is really most amazing change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2657:
URL: https://github.com/apache/helix/pull/2657#discussion_r1356887903
##########
helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java:
##########
@@ -389,6 +390,12 @@ public void setPreferenceLists(Map<String, List<String>> instanceLists) {
* @throws IOException - when JSON conversion fails
*/
public Map<String, Map<String, Integer>> getPartitionCapacityMap() throws IOException {
+ // It is very expensive to deserialize the partition capacity map every time this is called.
+ // Cache the deserialized map to avoid the overhead.
+ if (_deserializedPartitionCapacityMap != null) {
Review Comment:
we are relying on 'setPartitionCapacityMap' to reset the cache. Hopefully we are not calling setPartitinCapacityMap() too frequently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2657:
URL: https://github.com/apache/helix/pull/2657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on code in PR #2657:
URL: https://github.com/apache/helix/pull/2657#discussion_r1357268766
##########
helix-core/src/main/java/org/apache/helix/model/ResourceConfig.java:
##########
@@ -389,6 +390,12 @@ public void setPreferenceLists(Map<String, List<String>> instanceLists) {
* @throws IOException - when JSON conversion fails
*/
public Map<String, Map<String, Integer>> getPartitionCapacityMap() throws IOException {
+ // It is very expensive to deserialize the partition capacity map every time this is called.
+ // Cache the deserialized map to avoid the overhead.
+ if (_deserializedPartitionCapacityMap != null) {
Review Comment:
We do not call `setPartitionCapacityMap` anywhere but tests. Cache is refreshed by replacing ResourceConfig HelixProperty in dataProvider when there is a keyChange. We set _objMap to new HashMap<String, T> containing new value for keys that were changed, whenever we do the refresh. Replacing the property will ensure that _deserializedPartitionCapacityMap will be recomputed if ResourceConfig is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Add _deserializedPartitionCapacityMap to ResourceConfig model to ensure that the partition capacity map is only deserialized one time [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2657:
URL: https://github.com/apache/helix/pull/2657#issuecomment-1762204646
Looks like there is a flaky test that fails CI: testMissingTopStateDurationMonitoring
Locally, this test is passing:
```
mvn test -o -Dtest=TestControllerLeadershipChange -pl=helix-core
[INFO] Scanning for projects...
[WARNING]
[WARNING] Some problems were encountered while building the effective model for org.apache.helix:metrics-common:bundle:1.3.2-SNAPSHOT
[WARNING] 'build.pluginManagement.plugins.plugin.(groupId:artifactId)' must be unique but found duplicate declaration of plugin org.apache.maven.plugins:maven-javadoc-plugin @ org.apache.helix:helix:1.3.2-SNAPSHOT, /Users/zapinto/Documents/git2/zpinto/helix/pom.xml, line 678, column 17
[WARNING]
[WARNING] Some problems were encountered while building the effective model for org.apache.helix:helix:pom:1.3.2-SNAPSHOT
[WARNING] 'build.pluginManagement.plugins.plugin.(groupId:artifactId)' must be unique but found duplicate declaration of plugin org.apache.maven.plugins:maven-javadoc-plugin @ line 678, column 17
[WARNING]
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING]
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING]
[INFO]
[INFO] --------------------< org.apache.helix:helix-core >---------------------
[INFO] Building Apache Helix :: Core 1.3.2-SNAPSHOT
[INFO] from pom.xml
[INFO] -------------------------------[ bundle ]-------------------------------
[INFO]
[INFO] --- enforcer:1.4.1:enforce (enforce-maven-version) @ helix-core ---
[INFO]
[INFO] --- enforcer:1.4.1:enforce (enforce-java-version) @ helix-core ---
[INFO]
[INFO] --- enforcer:1.4.1:enforce (enforce-output-timestamp-property) @ helix-core ---
[INFO]
[INFO] --- jacoco:0.8.6:prepare-agent (default) @ helix-core ---
[INFO] argLine set to -javaagent:/Users/zapinto/.m2/repository/org/jacoco/org.jacoco.agent/0.8.6/org.jacoco.agent-0.8.6-runtime.jar=destfile=/Users/zapinto/Documents/git2/zpinto/helix/helix-core/target/jacoco.exec
[INFO]
[INFO] --- remote-resources:1.7.0:process (process-resource-bundles) @ helix-core ---
[INFO] Preparing remote bundle org.apache:apache-jar-resource-bundle:1.4
[INFO] Copying 3 resources from 1 bundle.
[INFO]
[INFO] --- resources:3.2.0:resources (default-resources) @ helix-core ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Using 'UTF-8' encoding to copy filtered properties files.
[INFO] Copying 4 resources
[INFO] Copying 0 resource
[INFO] Copying 3 resources
[INFO] The encoding used to copy filtered properties files have not been set. This means that the same encoding will be used to copy filtered properties files as when copying other filtered resources. This might not be what you want! Run your build with --debug to see which files might be affected. Read more at https://maven.apache.org/plugins/maven-resources-plugin/examples/filtering-properties-files.html
[INFO]
[INFO] --- compiler:3.10.1:compile (default-compile) @ helix-core ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 577 source files to /Users/zapinto/Documents/git2/zpinto/helix/helix-core/target/classes
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java: Some input files use or override a deprecated API.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java: Recompile with -Xlint:deprecation for details.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java: Some input files use unchecked or unsafe operations.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java: Recompile with -Xlint:unchecked for details.
[INFO]
[INFO] --- bundle:5.1.4:manifest (bundle-manifest) @ helix-core ---
[WARNING] Manifest org.apache.helix:helix-core:bundle:1.3.2-SNAPSHOT : Unused Import-Package instructions: [org.apache.logging.log4j*, org.apache.logging.slf4j*]
[INFO] Writing manifest: /Users/zapinto/Documents/git2/zpinto/helix/helix-core/target/classes/META-INF/MANIFEST.MF
[INFO]
[INFO] --- resources:3.2.0:testResources (default-testResources) @ helix-core ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Using 'UTF-8' encoding to copy filtered properties files.
[INFO] Copying 15 resources
[INFO] Copying 3 resources
[INFO]
[INFO] --- compiler:3.10.1:testCompile (default-testCompile) @ helix-core ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 498 source files to /Users/zapinto/Documents/git2/zpinto/helix/helix-core/target/test-classes
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/test/java/org/apache/helix/integration/TestCarryOverBadCurState.java: Some input files use or override a deprecated API.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/test/java/org/apache/helix/integration/TestCarryOverBadCurState.java: Recompile with -Xlint:deprecation for details.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java: Some input files use unchecked or unsafe operations.
[INFO] /Users/zapinto/Documents/git2/zpinto/helix/helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java: Recompile with -Xlint:unchecked for details.
[INFO]
[INFO] --- surefire:3.0.0-M3:test (default-test) @ helix-core ---
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.helix.integration.controller.TestControllerLeadershipChange
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 26.822 s - in org.apache.helix.integration.controller.TestControllerLeadershipChange
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/zapinto/Documents/git2/zpinto/helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 947 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 46.916 s
[INFO] Finished at: 2023-10-13T13:50:09-07:00
[INFO] ------------------------------------------------------------------------
```
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org