-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Backport 2.x] [Segment Replication] Backport PR's : #3525 #3533 #3540 #3943 #3963 From main branch #4181
[Backport 2.x] [Segment Replication] Backport PR's : #3525 #3533 #3540 #3943 #3963 From main branch #4181
Conversation
…oject#3525. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
…rdAlreadyReplicating() (opensearch-project#3943) * Fixing flaky test failure happening for testShardAlreadyReplicating() Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
…Replications() (opensearch-project#3963) * Fixing flaky test failure happening for testShardAlreadyReplicating() Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
…ard class. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## 2.x #4181 +/- ##
============================================
+ Coverage 70.45% 70.46% +0.01%
- Complexity 56690 56769 +79
============================================
Files 4563 4569 +6
Lines 273024 273348 +324
Branches 40059 40083 +24
============================================
+ Hits 192364 192628 +264
- Misses 64521 64524 +3
- Partials 16139 16196 +57
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -2657,7 +2708,6 @@ public long getLocalCheckpoint() { | |||
* Also see {@link #getLocalCheckpoint()}. | |||
*/ | |||
public long getProcessedLocalCheckpoint() { | |||
assert indexSettings.isSegRepEnabled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert has been removed because it is failing many unit tests in SegmentReplicationTargetServiceTests.java and SegmentReplicationIndexShardTests.java. The unit tests in these classes don't use index setting as SEGMENT, so when an assert is done on if segRepEnabled() all of the shards fails the assert and don't perform necessary unit tests.
This assert has been added to 2.x branch in this backport PR. As the original PR had method getProcessedLocalCheckpoint() in Engine.java as abstract which is a breaking. To avoid breaking breaking changes we made few changes to original PR while backporting. And this assert is part of those changes. It is not necessary to have this assert as it will any how be removed in 3.0 opensearch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the SEGMENT
index setting in test classes. Can we backport #4163 if it helps ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might help actually, but let me check by backporting your PR locally and add back assert statement to see if all of the unit tests pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding back assert and use SEGMENT replication as index setting in unit tests. But got this error:
REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.indices.replication.OngoingSegmentReplicationsTests.testMultipleReplicasUseSameCheckpoint" -Dtests.seed=A0971E691B2879E0 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ms-MY -Dtests.timezone=Asia/Kolkata -Druntime.java=17
2> java.lang.AssertionError
at __randomizedtesting.SeedInfo.seed([A0971E691B2879E0:A4046FDAE5B7632]:0)
at org.opensearch.index.shard.IndexShard.getProcessedLocalCheckpoint(IndexShard.java:2711)
at org.opensearch.indices.replication.OngoingSegmentReplicationsTests.setUp(OngoingSegmentReplicationsTests.java:73)
so there are multiple places we have to makes changes to make this work and I don't think this PR is right place to do all changes. We can do them while backporting other PR's. So for now I am removing assert statement
…ation in SegmentReplicationSourceHandlerTests and SegmentReplicationTargetServiceTests. Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
Flaky test:
Actually this is not flaky test. this happened due to changes in recent commit. Will fix this in next commit |
Another appearance in #4041. Created #4184 to track it as it is failing more oftenly on CI. |
Gradle Check (Jenkins) Run Completed with:
|
Sorry @dreamer-89 , I thought this is a flaky test but not. This happened because of adding "assert statement" back that we discussed in above comments. I will remove the assert to fix this |
…t replication in SegmentReplicationSourceHandlerTests and SegmentReplicationTargetServiceTests." Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com> This reverts commit 8c5753b.
Gradle Check (Jenkins) Run Completed with:
|
Description
This PR backports following 5 PR's from main branch:
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.