[Segment Replication] Fix SearchableSnapshotIT tests failing with segment replication enabled indices #7036
Conversation
Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
Rishikesh1159
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 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 #7036 +/- ##
============================================
+ Coverage 70.78% 70.82% +0.04%
- Complexity 59269 59314 +45
============================================
Files 4823 4823
Lines 283985 283985
Branches 40953 40954 +1
============================================
+ Hits 201026 201142 +116
+ Misses 66403 66321 -82
+ Partials 16556 16522 -34
... and 515 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| if (idxSettings.isSegRepEnabled()) { | ||
| return new NRTReplicationEngineFactory(); | ||
| } | ||
| if (idxSettings.isRemoteSnapshot()) { | ||
| return config -> new ReadOnlyEngine(config, new SeqNoStats(0, 0, 0), new TranslogStats(), true, Function.identity(), false); | ||
| } | ||
| if (idxSettings.isSegRepEnabled()) { | ||
| return new NRTReplicationEngineFactory(); | ||
| } |
There was a problem hiding this comment.
LGTM! Thank you @Rishikesh1159 for fixing this
|
Is there a plan to run all tests regularly with segment replication enabled? Or should we add a case to SearchableSnapshotIT the explicitly enables it in order to have a test that catches a failure like this? |
Not soon but in near future, we are planning to run all integ tests regularly with segment replication, like adding randomization in OpensearchIntegTest class that would make test classes to either use segrep or docrep randomly. But we still don't have a solid plan on how we should do it. Yeah we can also add a case for SearchableSnapshotIT the explicitly enables, but rather than duplicating it on every other test class it would be better if we can figure a way out to do it with in OpensearchIntegTest |
Description
When we are restoring snapshot with storage type as REMOTE_SNAPSHOT we should use ReadOnlyEngine. But currectly we are wiring up a NRTReplicationEngineFactor instead of
ReadOnlyEngine.This PR moves up the
isRemoteSnapshot()check on index setting before we check ifisSegRepEnabled(). This way be wireReadOnlyEnginewhen using remote snapshot.Issues Resolved
Resolves #7034
Check List
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.