Refactoring GatedAutoCloseable and moving RecoveryState.Timer #2965

kartg · 2022-04-18T22:22:59Z

Signed-off-by: Kartik Ganesh gkart@amazon.com

Description

This PR is one of the first steps in merging the feature branch feature/segment-replication back into main by re-PRing key changes. It covers part of the code changes in #2926 . The breakdown of the plan to merge segment-replication to main is detailed in #2355

The changes included in this PR are as follows:

GatedAutoCloseable currently wraps a subclass of RefCounted. Segment replication adds another subclass, but this also wraps RefCounted. Both subclasses have the same shutdown hook - decRef. Therefore, GatedAutoCloseable has been made less generic (and renamed to AutoCloseableRefCounted) to increase code convergence.
The RecoveryState.Timer inner class has been moved to a top-level class. This will eventually be reused across both replication use-cases - peer recovery and segment replication.
In the RecoveryTargetTests test class, the test methods for Timer classes have been separated into distinct unit tests for each subclass. Common test logic is shared via a private method. The use of RandomBoolean has been removed to avoid non-deterministic code paths in testing. While doing so, the usage of assertThat (deprecated) has been replaced in these code paths.
Finally, a minor change removes the readRecoveryState API from RecoveryState, replacing it by a direct invocation of the constructor.

Segment replication design proposal - #2229

Issues Resolved

N/A

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

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.

This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch. GatedAutoCloseable currently wraps a subclass of RefCounted. Segment replication adds another subclass, but this also wraps RefCounted. Both subclasses have the same shutdown hook - decRef. This change makes the superclass less generic to increase code convergence. The breakdown of the plan to merge segment-replication to main is detailed in opensearch-project#2355 Segment replication design proposal - opensearch-project#2229 Signed-off-by: Kartik Ganesh <gkart@amazon.com>

This change makes two minor updates to RecoveryState - 1. The readRecoveryState API is removed because it can be replaced by an invocation of the constructor 2. The class members of the Timer inner class are changed to private, and accesses are only through the public APIs Signed-off-by: Kartik Ganesh <gkart@amazon.com>

This change removes the use of RandomBoolean in testing the Timer classes and creates a dedicated unit test for each. The common test logic is shared via a private method. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

This will eventually be reused across both replication use-cases - peer recovery and segment replication. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

Removes a non-deterministic code path around stopping the timer, and avoids assertThat (deprecated) Signed-off-by: Kartik Ganesh <gkart@amazon.com>

opensearch-ci-bot · 2022-04-18T23:02:01Z

✅ Gradle Check success e5d632b
Log 4602

Reports 4602

andrross

Automated reviews are unhappy. Maybe missing a trailing newline?

andrross · 2022-04-19T22:40:27Z

server/src/main/java/org/opensearch/indices/replication/common/Timer.java

+ * A serializable timer that is used to measure the time taken for
+ * file replication operations like recovery.
+ */
+public class Timer implements Writeable {


"Timer" is a super generic name, used in lots of places (e.g. java.util.Timer, org.opensearch.search.profile.Timer, etc). Is there a more specific name that makes sense now that it is no longer an inner class? Maybe ReplicationTimer?

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

opensearch-ci-bot · 2022-04-20T02:04:37Z

❌ Gradle Check failure ee79dcc
Log 4633

Reports 4633

kartg · 2022-04-20T16:47:19Z

Failure due to a flaky assertion on a timer. I'm currently looking into how we can improve these tests.

org.opensearch.indices.recovery.RecoveryTargetTests > testIndexTimer FAILED
    java.lang.AssertionError: expected:<20> but was:<14>
        at __randomizedtesting.SeedInfo.seed([1AB462596BA6129E:29DE4F529849686D]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:633)
        at org.opensearch.indices.recovery.RecoveryTargetTests.doTimerTest(RecoveryTargetTests.java:180)
        at org.opensearch.indices.recovery.RecoveryTargetTests.testIndexTimer(RecoveryTargetTests.java:146)

Trying to serialize and deserialize a running Timer instance, and then checking for equality leads to flaky test failures when the ser/deser takes time. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

opensearch-ci-bot · 2022-04-20T18:23:53Z

❌ Gradle Check failure 97fa82e
Log 4643

Reports 4643

kartg · 2022-04-20T20:00:30Z

Flaky test failure #1703

Refiring gradle check

kartg · 2022-04-20T20:00:41Z

start gradle check

opensearch-ci-bot · 2022-04-20T20:30:06Z

✅ Gradle Check success 97fa82e
Log 4650

Reports 4650

dblock

This looks straightforward, thank you. Will leave it for @andrross to do another pass.

dblock · 2022-04-20T22:02:55Z

I labelled to backport to 2.x (2.1), nothing breaking here right?

* Refactoring GatedAutoCloseable to AutoCloseableRefCounted This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch. GatedAutoCloseable currently wraps a subclass of RefCounted. Segment replication adds another subclass, but this also wraps RefCounted. Both subclasses have the same shutdown hook - decRef. This change makes the superclass less generic to increase code convergence. The breakdown of the plan to merge segment-replication to main is detailed in #2355 Segment replication design proposal - #2229 Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Minor refactoring in RecoveryState This change makes two minor updates to RecoveryState - 1. The readRecoveryState API is removed because it can be replaced by an invocation of the constructor 2. The class members of the Timer inner class are changed to private, and accesses are only through the public APIs Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Update RecoveryTargetTests to test Timer subclasses deterministically This change removes the use of RandomBoolean in testing the Timer classes and creates a dedicated unit test for each. The common test logic is shared via a private method. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Move the RecoveryState.Timer class to a top-level class This will eventually be reused across both replication use-cases - peer recovery and segment replication. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Further update of timer tests in RecoveryTargetTests Removes a non-deterministic code path around stopping the timer, and avoids assertThat (deprecated) Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Rename to ReplicationTimer Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Remove RecoveryTargetTests assert on a running timer Trying to serialize and deserialize a running Timer instance, and then checking for equality leads to flaky test failures when the ser/deser takes time. Signed-off-by: Kartik Ganesh <gkart@amazon.com> (cherry picked from commit c7c410a)

kartg · 2022-04-21T00:49:51Z

I labelled to backport to 2.x (2.1), nothing breaking here right?

Correct, nothing breaking; safe to backport to 2.x

…#3014) * Refactoring GatedAutoCloseable to AutoCloseableRefCounted This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch. GatedAutoCloseable currently wraps a subclass of RefCounted. Segment replication adds another subclass, but this also wraps RefCounted. Both subclasses have the same shutdown hook - decRef. This change makes the superclass less generic to increase code convergence. The breakdown of the plan to merge segment-replication to main is detailed in #2355 Segment replication design proposal - #2229 Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Minor refactoring in RecoveryState This change makes two minor updates to RecoveryState - 1. The readRecoveryState API is removed because it can be replaced by an invocation of the constructor 2. The class members of the Timer inner class are changed to private, and accesses are only through the public APIs Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Update RecoveryTargetTests to test Timer subclasses deterministically This change removes the use of RandomBoolean in testing the Timer classes and creates a dedicated unit test for each. The common test logic is shared via a private method. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Move the RecoveryState.Timer class to a top-level class This will eventually be reused across both replication use-cases - peer recovery and segment replication. Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Further update of timer tests in RecoveryTargetTests Removes a non-deterministic code path around stopping the timer, and avoids assertThat (deprecated) Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Rename to ReplicationTimer Signed-off-by: Kartik Ganesh <gkart@amazon.com> * Remove RecoveryTargetTests assert on a running timer Trying to serialize and deserialize a running Timer instance, and then checking for equality leads to flaky test failures when the ser/deser takes time. Signed-off-by: Kartik Ganesh <gkart@amazon.com> (cherry picked from commit c7c410a) Co-authored-by: Kartik Ganesh <gkart@amazon.com>

kartg added 5 commits April 18, 2022 14:55

Move the RecoveryState.Timer class to a top-level class

948bb69

This will eventually be reused across both replication use-cases - peer recovery and segment replication. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

Further update of timer tests in RecoveryTargetTests

e5d632b

Removes a non-deterministic code path around stopping the timer, and avoids assertThat (deprecated) Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg requested a review from a team as a code owner April 18, 2022 22:22

kartg mentioned this pull request Apr 18, 2022

[Segment Replication] Merge to main tracking - Refactoring of existing classes to support segrep. #2926

Closed

5 tasks

andrross reviewed Apr 19, 2022

View reviewed changes

Rename to ReplicationTimer

ee79dcc

Signed-off-by: Kartik Ganesh <gkart@amazon.com>

kartg requested a review from reta as a code owner April 20, 2022 01:41

Remove RecoveryTargetTests assert on a running timer

97fa82e

Trying to serialize and deserialize a running Timer instance, and then checking for equality leads to flaky test failures when the ser/deser takes time. Signed-off-by: Kartik Ganesh <gkart@amazon.com>

dblock requested a review from nknize April 20, 2022 21:15

dblock approved these changes Apr 20, 2022

View reviewed changes

dblock removed the request for review from nknize April 20, 2022 21:18

andrross approved these changes Apr 20, 2022

View reviewed changes

dblock merged commit c7c410a into opensearch-project:main Apr 20, 2022

dblock added the backport 2.x Backport to 2.x branch label Apr 20, 2022

opensearch-trigger-bot bot mentioned this pull request Apr 20, 2022

[Backport 2.x] Refactoring GatedAutoCloseable and moving RecoveryState.Timer #3014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring GatedAutoCloseable and moving RecoveryState.Timer #2965

Refactoring GatedAutoCloseable and moving RecoveryState.Timer #2965

kartg commented Apr 18, 2022 •

edited

Loading

opensearch-ci-bot commented Apr 18, 2022

andrross left a comment

andrross Apr 19, 2022

opensearch-ci-bot commented Apr 20, 2022

kartg commented Apr 20, 2022

opensearch-ci-bot commented Apr 20, 2022

kartg commented Apr 20, 2022

kartg commented Apr 20, 2022

opensearch-ci-bot commented Apr 20, 2022

dblock left a comment •

edited

Loading

dblock commented Apr 20, 2022

kartg commented Apr 21, 2022

Refactoring GatedAutoCloseable and moving RecoveryState.Timer #2965

Refactoring GatedAutoCloseable and moving RecoveryState.Timer #2965

Conversation

kartg commented Apr 18, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Apr 18, 2022

andrross left a comment

Choose a reason for hiding this comment

andrross Apr 19, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Apr 20, 2022

kartg commented Apr 20, 2022

opensearch-ci-bot commented Apr 20, 2022

kartg commented Apr 20, 2022

kartg commented Apr 20, 2022

opensearch-ci-bot commented Apr 20, 2022

dblock left a comment • edited Loading

Choose a reason for hiding this comment

dblock commented Apr 20, 2022

kartg commented Apr 21, 2022

kartg commented Apr 18, 2022 •

edited

Loading

dblock left a comment •

edited

Loading