Add a new Engine implementation for replicas with segment replication enabled.

[mch2]

Description

This change adds a new engine NRTReplicationEngine intended to be used on replicas with segment replication enabled.

With segrep enabled replicas should not have a configured IndexWriter but we should still write operations to a translog.
In order to do this, this change pulls out common translog functionality from InternalEngine into a base class TranslogAwareEngine that is extended by both internalEngine and NRTReplicationEngine.

The engine uses a new OpensearchReaderManager NRTReplicationReaderManager that refreshes from an externally provided SegmentInfos. This will be refreshed after new segments have been received.

This PR includes a new engine factory that will be used only if segrep is enabled. The factory uses an NRTReplicationEngine for replicas and InternalEngine for primaries.

This PR also removes the upper bound check in LocalCheckpointTracker'sfastForwardProcessedSeqNo. The persisted seqNo will be fast forwarded when a set of segments is received, however this is not coupled a xlog sync.

Issues Resolved

closes #3109

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.

[mch2]

@Bukhtawar I'm curious what you think about the translog changes here and how it may relate to pluggable translog? I'm thinking the base class would use the TranslogPlugin that is in your proposal.

[opensearch-ci-bot]

❌   Gradle Check failure e77b616
Log 5140

Reports 5140

[opensearch-ci-bot]

❌   Gradle Check failure 22e7dde
Log 5141

Reports 5141

[opensearch-ci-bot]

❌   Gradle Check failure daacef4
Log 5142

Reports 5142

[opensearch-ci-bot]

❌   Gradle Check failure 3d3cf48
Log 5143

Reports 5143

[mch2]

start gradle check

[opensearch-ci-bot]

❌   Gradle Check failure 3d3cf48
Log 5169

Reports 5169

[Bukhtawar]

Thanks @mch2 I guess we are trying to abstract out the translog specific logic to a common place so that it could be reused across primary and replica. With Pluggable translog #3241 we also intend to decouple the translog logic away from the Engine so that the Engine contracts do not mandate a Translog dependency

[mch2]

./gradlew ':qa:rolling-upgrade:v2.1.0#upgradedClusterTest' --tests "org.opensearch.upgrades.RecoveryIT.testRecoveryWithConcurrentIndexing" -Dtests.seed=425F891771F2E979 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=th -Dtests.timezone=America/Fort_Nelson -Druntime.java=17

Not sure yet if this is related or flaky...

[mch2]

Looks like a flaky test, retrying

[mch2]

start gradle check

[mch2]

Thanks @mch2 I guess we are trying to abstract out the translog specific logic to a common place so that it could be reused across primary and replica. With Pluggable translog #3241 we also intend to decouple the translog logic away from the Engine so that the Engine contracts do not mandate a Translog dependency

Thanks @Bukhtawar. That decoupling makes sense, do you have any concerns with this acting as a stepping stone? I didn't want to start ripping up the Engine contract. I could also perhaps pull this into a delegate class instead of inheritance.

[opensearch-ci-bot]

✅   Gradle Check success 3d3cf48
Log 5182

Reports 5182

[Bukhtawar]

While this certainly serves as a stepping stone, in the way it extracts the translog functionality into a separate class, the eventual goal would be to have the Engine composed of a Translog which would have a no-op implementation for a client managed durability semantics. I am fine with the current change, the decoupling could be taken up in a separate PR

[nknize]

hrmm... this is a pretty large change as it is.. but all it really does is move translog deck chairs to an intermediate TranslogAwareEngine removing InternalEngine one class further from the Engine Base class (which still has translog specific abstract methods). I'm not sure it's really worth doing this stepping stone change than just creating a NoOp translog? Maybe first start by creating a NoOpSnapshot implements Translog.Snapshot in the spirit of LuceneChangesSnapshot? Going that direction might minimize the blast radius?

[mch2]

hrmm... this is a pretty large change as it is.. but all it really does is move translog deck chairs to an intermediate TranslogAwareEngine removing InternalEngine one class further from the Engine Base class (which still has translog specific abstract methods). I'm not sure it's really worth doing this stepping stone change than just creating a NoOp translog? Maybe first start by creating a NoOpSnapshot implements Translog.Snapshot in the spirit of LuceneChangesSnapshot? Going that direction might minimize the blast radius?

Yeah the TranslogAwareEngine inheritence was intended as a way for reuse across both the segrep engine and existing InternalEngine because we'll still need to write to the xlog on replica with segrep. Though I can see how TranslogAwareEngine implies we have Engine implementations that do not write to an xlog, which isn't the case. Perhaps a better way forward here is to ignore the xlog writes initially for the segrep engine until we have that pluggability or implement a NoOp version of the TranslogPlugin interface outlined in #1319, and inject that into segrep engine.

[mch2]

The Engine design is flawed. There really should only be one Engine with components that can be configured based on need (e.g, ReadOnly); and Plugins should never have been able to override the entire engine. This is horribly trappy and just a terrible idea. Plugins should be able to add functionality to the singular engine in places that make sense and are safely opened by the core (e.g., adding a custom encoding format to the CodecService or tweaking a translog deletion policy). Not to mention, there is no graceful handling of overriding the engine! Any plugin can override the entire thing with it's own custom implementation and a give up IllegalStateException is thrown if more than one plugin provides it's own EngineFactory! Terrible.

...simply a passthrough to a translog method with engine specific handling

Yep! Horrible. There shouldn't be "low level" abstract translog methods in the engine (e.g., rollTranslogGeneration). I think Translog behavior should be isolated to a TranslogHandler that the engine can hold a reference to and call if/when needed. This would allow greater flexibility in the translog such that we could plugin different configurations and implementations. This is what I mean by a "NoOp" Translog. If we have a translog handler in the engine then we can delegate those abstract method calls to the handler and further decouple that translog logic from the core engine (e.g., instead of engine.rollTranslogGeneration(); in IndexShard it should be getEngine().translogHandler().rollGeneration();). Not only would this would be a cleaner change, it would get us closer to complete decoupling of the translog and prevent further fragmenting the Engine by adding another custom implementation.

Agree. Ok with me following up on this in a separate change as part of #3241 ?

[opensearch-ci-bot]

❌   Gradle Check failure 5bccdd8
Log 5265

Reports 5265

[opensearch-ci-bot]

✅   Gradle Check success 5b380d2
Log 5267

Reports 5267

[nknize]

Ok with me following up on this in a separate change as part of #3241 ?

Yes

[nknize]

I'm good w/ this in main as a stepping stone until we decouple xlog and slim to one Engine (w/ readOnly mode available).

[nknize]

/cc @reta @Bukhtawar Giving a quick 24hr notice on this. I'm okay w/ it as a stepping stone to moving toward a single engine w/ pluggable xlog but let us know if you have any major concerns.

[Bukhtawar]

We would need some handling to ensure translog rolls over, I am guessing this would be a part of checkpoint refresh?

[mch2]

Good catch this is missing. I think this would happen when a replica receives new segments belonging to a new commit point, indicating the primary has flushed, will add a test.

[mch2]

updated to roll xlog when new commit point is received.

[opensearch-ci-bot]

✅   Gradle Check success 06b0d98
Log 5466

Reports 5466

Requested changes have been made.

[mch2]

@Bukhtawar I've made the updates requested here. Please let me know if there are any major concerns, would like to move fwd with this. Thanks.

[Bukhtawar]

Ack on it

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3240-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a0030dfb47b4312eb4087f17cc3cee0139c74b6a
# Push it to GitHub
git push --set-upstream origin backport/backport-3240-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3240-to-2.x.