Fix a bug in Search Backpressure #6455

PritLadani · 2023-02-23T08:07:53Z

Description

Bug-fix in Search Backpressure's HeapUsageTracker.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: PritLadani pritkladani@gmail.com

Signed-off-by: PritLadani <pritkladani@gmail.com>

github-actions · 2023-02-23T08:38:38Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11626/
CommitID: 7e76ecd

codecov-commenter · 2023-02-23T08:41:29Z

Codecov Report

Merging #6455 (7e76ecd) into main (4dd0c5f) will increase coverage by 0.07%.
The diff coverage is 90.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6455      +/-   ##
============================================
+ Coverage     70.76%   70.83%   +0.07%     
+ Complexity    59093    59068      -25     
============================================
  Files          4802     4800       -2     
  Lines        282793   282712      -81     
  Branches      40782    40763      -19     
============================================
+ Hits         200111   200261     +150     
+ Misses        66271    66003     -268     
- Partials      16411    16448      +37

Impacted Files	Coverage Δ
...va/org/opensearch/cluster/routing/RoutingNode.java	`78.53% <ø> (-0.26%)`	⬇️
...ting/allocation/allocator/LocalShardsBalancer.java	`84.90% <ø> (-0.33%)`	⬇️
...r/routing/allocation/allocator/ShardsBalancer.java	`33.33% <ø> (+8.33%)`	⬆️
...search/common/settings/AbstractScopedSettings.java	`88.38% <ø> (+1.25%)`	⬆️
...rg/opensearch/common/settings/ClusterSettings.java	`91.89% <ø> (ø)`
...n/java/org/opensearch/common/settings/Setting.java	`86.13% <ø> (-0.22%)`	⬇️
.../allocation/allocator/BalancedShardsAllocator.java	`91.17% <85.71%> (+0.60%)`	⬆️
...egations/support/MultiTermsValuesSourceConfig.java	`80.32% <100.00%> (ø)`
...egations/support/MultiValuesSourceFieldConfig.java	`92.50% <100.00%> (ø)`
...search/backpressure/trackers/HeapUsageTracker.java	`68.96% <100.00%> (+3.44%)`	⬆️
... and 493 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: PritLadani <pritkladani@gmail.com>

github-actions · 2023-02-23T09:46:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/11629/
CommitID: bce2756

PritLadani · 2023-02-23T11:00:49Z

@reta can you please help to merge this small bug fix CR?

gbbafna · 2023-02-23T11:17:25Z

CHANGELOG.md

@@ -70,6 +70,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ## [Unreleased 2.x]
 ### Added
 - Add GeoTile and GeoHash Grid aggregations on GeoShapes. ([#5589](https://github.com/opensearch-project/OpenSearch/pull/5589))
+- Small bug fix in Search Backpressure's HeapUsageTracker ([#6455](https://github.com/opensearch-project/OpenSearch/pull/6455))


For small non-customer facing changes , we can skip the changelog.

okay, removing it.

reta · 2023-02-23T12:01:55Z

@reta can you please help to merge this small bug fix CR?

@PritLadani my apologies, I am on vacation the rest of the week

This reverts commit bce2756.

github-actions · 2023-02-23T12:56:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      2 org.opensearch.cluster.service.MasterServiceTests.classMethod
      1 org.opensearch.cluster.service.MasterServiceTests.testThrottlingForMultipleTaskTypes

URL: https://build.ci.opensearch.org/job/gradle-check/11631/
CommitID: b33c490
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: PritLadani <pritkladani@gmail.com>

dreamer-89 · 2023-02-23T16:25:40Z

server/src/main/java/org/opensearch/search/backpressure/trackers/HeapUsageTracker.java

@@ -107,7 +107,7 @@ public static boolean isHeapTrackingSupported() {
     */
    public static boolean isHeapUsageDominatedBySearch(List<CancellableTask> cancellableTasks, double heapPercentThreshold) {
        long usage = cancellableTasks.stream().mapToLong(task -> task.getTotalResourceStats().getMemoryInBytes()).sum();
-        long threshold = (long) heapPercentThreshold * HEAP_SIZE_BYTES;
+        long threshold = (long) (heapPercentThreshold * HEAP_SIZE_BYTES);


nit: Good to have one unit test which verifies this change

@PritLadani Can you add unit tests here in a follow up PR? This is a public static method and should be pretty straightforward to unit test.

Done here. #6463
Thanks!

github-actions · 2023-02-23T16:36:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testNewlyAddedReplicaIsUpdated

URL: https://build.ci.opensearch.org/job/gradle-check/11633/
CommitID: 9256ef7
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

dreamer-89 · 2023-02-23T16:47:34Z

testNewlyAddedReplicaIsUpdated

Tracking in #6459

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.indices.replication.SegmentReplicationRelocationIT.testNewlyAddedReplicaIsUpdated" -Dtests.seed=A3D21C76F7A36009 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-CU -Dtests.timezone=Africa/Algiers -Druntime.java=19

org.opensearch.indices.replication.SegmentReplicationRelocationIT > testNewlyAddedReplicaIsUpdated FAILED
    java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([A3D21C76F7A36009:E5CCE64E0859A9D0]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertFalse(Assert.java:65)
        at org.junit.Assert.assertFalse(Assert.java:75)
        at org.opensearch.indices.replication.SegmentReplicationRelocationIT.testNewlyAddedReplicaIsUpdated(SegmentReplicationRelocationIT.java:432)

andrross · 2023-02-23T16:49:16Z

Can you provide a more descriptive commit message/PR title? Something like "Fix integer math bug in HeapUsageTracker"

dreamer-89 · 2023-02-23T16:52:39Z

@PritLadani : The DCO check is failing right now. Can you please fix this

kartg · 2023-02-23T17:07:40Z

@PritLadani : The DCO check is failing right now. Can you please fix this

@dreamer-89 looks like the offending commit is the revert. Since we're trying to merge this as a critical bugfix for the 2.6 release, I'll go ahead and set the DCO check to pass and merge this in. Let me know if you've got any concerns.

andrross · 2023-02-23T17:07:42Z

@PritLadani Can you add unit tests here in a follow up PR? This is a public static method and should be pretty straightforward to unit test.

* Fix a bug in Search Backpressure Signed-off-by: PritLadani <pritkladani@gmail.com> --------- Signed-off-by: PritLadani <pritkladani@gmail.com> (cherry picked from commit d439244) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Fix a bug in Search Backpressure --------- (cherry picked from commit d439244) Signed-off-by: PritLadani <pritkladani@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Fix a bug in Search Backpressure

7e76ecd

Signed-off-by: PritLadani <pritkladani@gmail.com>

ketanv3 approved these changes Feb 23, 2023

View reviewed changes

nssuresh2007 approved these changes Feb 23, 2023

View reviewed changes

Adding changelog entry

bce2756

Signed-off-by: PritLadani <pritkladani@gmail.com>

gbbafna reviewed Feb 23, 2023

View reviewed changes

Revert "Adding changelog entry"

b33c490

This reverts commit bce2756.

PritLadani requested a review from gbbafna February 23, 2023 12:27

gbbafna added the skip-changelog label Feb 23, 2023

gbbafna approved these changes Feb 23, 2023

View reviewed changes

Empty commit

9256ef7

Signed-off-by: PritLadani <pritkladani@gmail.com>

dreamer-89 reviewed Feb 23, 2023

View reviewed changes

dreamer-89 mentioned this pull request Feb 23, 2023

[Segment Replication] SegmentReplicationRelocationIT.testNewlyAddedReplicaIsUpdated flaky test failure. #6459

Closed

kartg merged commit d439244 into opensearch-project:main Feb 23, 2023

kartg added backport 2.x Backport to 2.x branch backport 2.6 Backport to 2.6 branch labels Feb 23, 2023

This was referenced Feb 23, 2023

[Backport 2.6] Fix integer math bug in HeapUsageTracker #6460

Merged

[Backport 2.x] Fix integer math bug in HeapUsageTracker #6461

Merged

kartg mentioned this pull request Feb 23, 2023

Server side Cancellation of in-flight search requests based on resource consumption #1181

Open

kartg mentioned this pull request Feb 24, 2023

[Testing Confirmation] Confirm current testing requirements #6389

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug in Search Backpressure #6455

Fix a bug in Search Backpressure #6455

PritLadani commented Feb 23, 2023 •

edited

Loading

github-actions bot commented Feb 23, 2023

codecov-commenter commented Feb 23, 2023

github-actions bot commented Feb 23, 2023

PritLadani commented Feb 23, 2023

gbbafna Feb 23, 2023

PritLadani Feb 23, 2023

reta commented Feb 23, 2023

github-actions bot commented Feb 23, 2023

dreamer-89 Feb 23, 2023 •

edited

Loading

PritLadani Feb 23, 2023

github-actions bot commented Feb 23, 2023

dreamer-89 commented Feb 23, 2023

andrross commented Feb 23, 2023

dreamer-89 commented Feb 23, 2023

kartg commented Feb 23, 2023

andrross commented Feb 23, 2023

Fix a bug in Search Backpressure #6455

Fix a bug in Search Backpressure #6455

Conversation

PritLadani commented Feb 23, 2023 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Feb 23, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Feb 23, 2023

Codecov Report

github-actions bot commented Feb 23, 2023

Gradle Check (Jenkins) Run Completed with:

PritLadani commented Feb 23, 2023

gbbafna Feb 23, 2023

Choose a reason for hiding this comment

PritLadani Feb 23, 2023

Choose a reason for hiding this comment

reta commented Feb 23, 2023

github-actions bot commented Feb 23, 2023

Gradle Check (Jenkins) Run Completed with:

dreamer-89 Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

PritLadani Feb 23, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2023

Gradle Check (Jenkins) Run Completed with:

dreamer-89 commented Feb 23, 2023

andrross commented Feb 23, 2023

dreamer-89 commented Feb 23, 2023

kartg commented Feb 23, 2023

andrross commented Feb 23, 2023

PritLadani commented Feb 23, 2023 •

edited

Loading

dreamer-89 Feb 23, 2023 •

edited

Loading