Skip to content
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

Removing String format in RemoteStoreMigrationAllocationDecider #14612

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

RS146BIJAY
Copy link
Contributor

@RS146BIJAY RS146BIJAY commented Jul 1, 2024

Description

String.format is a relatively expensive operation for concatenating strings. Inside RemoteStoreMigrationAllocationDecider, the function calls to String.format stands out as the most resource-intensive operation in terms of latency.

Screenshot 2024-06-27 at 7 04 26 PM

This PR improves the latency of RemoteStoreMigrationAllocationDecider by replacing calls to String.format with more efficient StringBuilder for string concatenation.

Latency improvements

For initialising 225K shards on 500 nodes, we are observing an improvement of 31 seconds with above change.

Before the change

391,940.31 ms

After the change

360,279.58 ms

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@RS146BIJAY RS146BIJAY changed the title Remvoing String format in RemoteStoreMigrationAllocationDecider Removing String format in RemoteStoreMigrationAllocationDecider Jul 1, 2024
Copy link
Contributor

github-actions bot commented Jul 1, 2024

❌ Gradle check result for 790919f: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jul 1, 2024

❌ Gradle check result for 4232c37: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jul 1, 2024

❌ Gradle check result for a41da6a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jul 2, 2024

❌ Gradle check result for f74b597: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

github-actions bot commented Jul 2, 2024

❌ Gradle check result for 01cc8cd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
Copy link
Contributor

github-actions bot commented Jul 2, 2024

❕ Gradle check result for 3e4fd1e: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.78%. Comparing base (6267e94) to head (3e4fd1e).
Report is 2 commits behind head on main.

Files Patch % Lines
...decider/RemoteStoreMigrationAllocationDecider.java 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #14612      +/-   ##
============================================
+ Coverage     71.65%   71.78%   +0.12%     
- Complexity    62272    62323      +51     
============================================
  Files          5140     5140              
  Lines        293020   293028       +8     
  Branches      42347    42347              
============================================
+ Hits         209977   210354     +377     
+ Misses        65766    65327     -439     
- Partials      17277    17347      +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

Thanks, can you please point me to a test that validates the messages

@RS146BIJAY
Copy link
Contributor Author

Thanks, can you please point me to a test that validates the messages

@Bukhtawar This is the unit test which validates this message:

"[remote_store migration_direction]: primary shard copy can not be allocated to a non-remote node",

@Bukhtawar Bukhtawar added the backport 2.x Backport to 2.x branch label Jul 3, 2024
@Bukhtawar Bukhtawar merged commit e82b432 into opensearch-project:main Jul 3, 2024
56 of 57 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 3, 2024
…timise performance(#14612)

Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
(cherry picked from commit e82b432)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Jul 3, 2024
…timise performance(opensearch-project#14612)

Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants