Refactor to move prepareIndex and prepareDelete methods to the Engine #19551
Conversation
github-actions
bot
added
enhancement
varunbharadwaj
force-pushed
the
vb/enginerefactor
branch
2 times, most recently
from
4aeb16d to
ece97de
Compare
|
❌ Gradle check result for ece97de: 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? |
|
❕ Gradle check result for ece97de: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
varunbharadwaj
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #19551 +/- ##
============================================
- Coverage 73.01% 73.00% -0.01%
+ Complexity 70546 70527 -19
============================================
Files 5719 5719
Lines 323251 323260 +9
Branches 46815 46816 +1
============================================
- Hits 236016 235993 -23
+ Misses 68226 68176 -50
- Partials 19009 19091 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
server/src/main/java/org/opensearch/index/shard/IndexShard.java
Outdated
Show resolved
Hide resolved
…rd to Engine Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
varunbharadwaj
force-pushed
the
vb/enginerefactor
branch
from
ece97de to
9659143
Compare
Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
varunbharadwaj
force-pushed
the
vb/enginerefactor
branch
from
9659143 to
15a5a5b
Compare
|
❌ Gradle check result for 15a5a5b: 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? |
| public Engine.Index prepareIndex( | ||
| DocumentMapperForType docMapper, | ||
| SourceToParse source, | ||
| long seqNo, | ||
| long primaryTerm, | ||
| long version, | ||
| VersionType versionType, | ||
| Engine.Operation.Origin origin, | ||
| long autoGeneratedIdTimestamp, | ||
| boolean isRetry, | ||
| long ifSeqNo, | ||
| long ifPrimaryTerm | ||
| ) { |
There was a problem hiding this comment.
The concern with this change is implementations of the Engine interface like the NRTReplicationEngine now gets this method while in practise it isn't supposed to index any document. Thoughts @mch2 ?
Do we need to override this method appropriately?
There was a problem hiding this comment.
for NRT specifically prepareIndex is never invoked -
.but we can take this opportunity to refactor more and remove that block from indexshard and push the noOp to NRT engine directly.
There was a problem hiding this comment.
Agree this isn't getting used but always good to be explicit for future use cases
…opensearch-project#19551) --------- Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
…opensearch-project#19551) --------- Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
…opensearch-project#19551) --------- Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com> Signed-off-by: Gagan Singh Saini <gagasa@amazon.com>
…opensearch-project#19551) --------- Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
…opensearch-project#19551) --------- Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
4 participants
Description
This PR refactors IndexShard and Engine to move
prepareIndexandprepareDeleteto the abstract Engine class to enable custom engine implementations to bypass OS mapping framework. This has benefits as described in #19550Related Issues
Resolves #19550
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.