Integrate Engine with decoupled Translog interfaces #3671

Bukhtawar · 2022-06-23T19:37:02Z

Signed-off-by: Bukhtawar Khan bukhtawa@amazon.com

Description

The PR aims to Integrate Engine with decoupled Translog interfaces introduced in #3638 . This is the second part of introducing new interfaces and implementation as a part of decoupling. The overall proposal can be reviewed at #3471
Breaks down the main decouple PR into #3471 Pluggable translog work

Issues Resolved

#3241

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.

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

opensearch-ci-bot · 2022-06-24T12:38:32Z

✅ Gradle Check success 20772d8
Log 6295

Reports 6295

Bukhtawar · 2022-06-24T12:52:03Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

+                translogManagerRef = new InternalTranslogManager(
+                    engineConfig.getTranslogConfig(),
+                    engineConfig.getPrimaryTermSupplier(),
+                    engineConfig.getGlobalCheckpointSupplier(),
+                    translogDeletionPolicy,
+                    shardId,
+                    readLock,
+                    () -> getLocalCheckpointTracker(),
+                    translogUUID,
+                    new CompositeTranslogEventListener(Arrays.asList(internalTranslogEventListener, translogEventListener)),
+                    this::ensureOpen
+                );


Contemplating introducing a TranslogFactory

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

opensearch-ci-bot · 2022-06-24T14:54:48Z

❌ Gradle Check failure a3d45e8aa43e96c8fba5329be196d2562d4dbda5
Log 6301

Reports 6301

opensearch-ci-bot · 2022-06-24T15:24:05Z

✅ Gradle Check success dc1d0c7
Log 6302

Reports 6302

reta · 2022-06-24T15:36:46Z

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

        } catch (IOException e) {
-            IOUtils.closeWhileHandlingException(store::decRef, readerManager);
+            Translog translog = null;
+            if (translogManagerRef != null) {


I am wondering if TranslogManager should implement Closeable as well since it manages closeable resource (like Translog), wdyt?

Thats a good point however there are multiple places where the Engine is consuming the underlying Translog directly, which I plan on getting rid of in subsequent PRs as a part of moving Translog to a module(this PR would become too big to review otherwise).

OpenSearch/server/src/main/java/org/opensearch/index/translog/TranslogManager.java

Line 102 in 304d830

Translog getTranslog(boolean ensureOpen);

Once complete I will make the suggested change

Opened #3709

reta · 2022-06-27T14:17:35Z

server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java

    @Override
    public IndexResult index(Index index) throws IOException {
        ensureOpen();
        IndexResult indexResult = new IndexResult(index.version(), index.primaryTerm(), index.seqNo(), false);
-        final Translog.Location location = translog.add(new Translog.Index(index, indexResult));
+        final Translog.Location location = translogManager.getTranslog(false).add(new Translog.Index(index, indexResult));


It was not clear to me at the first place why would getTranslog use boolean argument to check if engine is open or not: in all flows this value is set to false (please correct me if I am wrong). It seems like only tests use true as an argument (which could use different assertion for that). I would suggested to clean up interface by removing this confusing parameter.

Thanks @reta will make the change, let me know if you have more comments and I can try address them in the next revision

Thanks @Bukhtawar I went through a few times, I have nothing else to comment on

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar · 2022-06-28T13:02:46Z

start gradle check

reta · 2022-06-28T14:17:37Z

server/src/main/java/org/opensearch/index/translog/NoOpTranslogManager.java

@@ -93,7 +93,7 @@ public boolean shouldRollTranslogGeneration() {
    public void trimOperationsFromTranslog(long belowTerm, long aboveSeqNo) throws TranslogException {}

    @Override
-    public Translog getTranslog(boolean ensureOpen) {
+    public Translog getTranslog() {


FYI (new change set), the NoOpTranslogManager is effectively unusable since every single caller assumes it will never return null for Translog getTranslog(). Hopefully, NoOpTranslogManager will never be used but you may think about introducing NoOpTranslog

That's correct the NoOpTranslog is already being worked upon as a part of #3600.

Bukhtawar · 2022-06-28T14:48:26Z

start gradle check

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

nknize

Thanks for doing this! It's great to see the decoupling work begin. I gave it a quick review. I noticed a lot of this is cosmetic so I just have some validating questions.

One general question. Is the point of having TranslogManager as an interface because we want to be able to create other translog managers (e.g., kafka manager)?

nknize · 2022-06-28T15:04:48Z

server/src/main/java/org/opensearch/index/engine/Engine.java

@@ -167,6 +168,8 @@ public final EngineConfig config() {
        return engineConfig;
    }

+    public abstract TranslogManager translogManager();


🎉 Great to see this decoupling

nknize · 2022-06-28T15:10:56Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

@@ -308,7 +334,9 @@ public InternalEngine(EngineConfig engineConfig) {
            }
            this.lastRefreshedCheckpointListener = new LastRefreshedCheckpointListener(localCheckpointTracker.getProcessedCheckpoint());
            this.internalReaderManager.addListener(lastRefreshedCheckpointListener);
-            maxSeqNoOfUpdatesOrDeletes = new AtomicLong(SequenceNumbers.max(localCheckpointTracker.getMaxSeqNo(), translog.getMaxSeqNo()));
+            maxSeqNoOfUpdatesOrDeletes = new AtomicLong(
+                SequenceNumbers.max(localCheckpointTracker.getMaxSeqNo(), translogManager.getTranslog().getMaxSeqNo())


Not a big fan of this indirection. I think refactoring utility methods like getMaxSeqNo directly to the TranslogManager instead of invoking getTranslog everywhere is cleaner?

Update: I just noticed the previous review comments. Looks like you're planning to decouple this further in follow up PRs. So disregard.

nknize · 2022-06-28T15:19:39Z

server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java

@@ -759,20 +761,20 @@ public long getProcessedCheckpoint() {
    }

    public void testFlushIsDisabledDuringTranslogRecovery() throws IOException {
-        engine.ensureCanFlush(); // recovered already
+        engine.translogManager().ensureCanFlush(); // recovered already


This this indirection to translogManager() temporary and planned to be cleaned up in the follow on PRs?

Bukhtawar · 2022-06-28T15:38:22Z

One general question. Is the point of having TranslogManager as an interface because we want to be able to create other translog managers (e.g., kafka manager)?

Thanks @nknize
The idea was to free Engine of the translog responsibilities and create various flavours like NoOpTranslogManager to be able to create an Engine which doesn't support translog operations, WriteOnlyTranslogManager for NRTReplicaEngine.
The first part of the PR was #3638 where we introduced newer interfaces. For remote extensions(kafka) we have #3242 which is being worked upon. The idea there is to further decouple TranslogWriter and TranslogReader from its underlying store. I will be raising that PR soon.

peterzhuamazon · 2022-06-28T18:09:00Z

Hi @Bukhtawar
Gradle check fixed. Sorry for confusion.
I rebased your branch and pushed so PR re-run checks now, thanks.

nknize · 2022-06-28T18:37:48Z

Is the point of having TranslogManager as an interface because we want to be able to create other translog managers (e.g., kafka manager)?

The idea was to free Engine of the translog responsibilities and create various flavours

so, yes? 😄

For remote extensions(kafka) we have #3242 which is being worked upon.

I presume this will create a new KafkaTranslogManager?

I'm trying to glean from this incremental PR if the implementation design is to have 1:1 concrete TranslogManager implementations per translog mechanism. So if a user wants to use Kafka as the Translog we, OpenSearch, would provide a KafkaTranslogManager for handling the kafka specific logic?

mch2

Have only a nit, lgtm.

mch2 · 2022-06-28T22:31:41Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

+
+                    @Override
+                    public void onTragicFailure(AlreadyClosedException ex) {
+                        failOnTragicEvent(ex);


this is a nit - I'm wondering if we should leave it up to the engine to determine if the error is tragic or not instead of explicitly catching AlreadyClosedException in InternalTranslogManager? Then we don't need this separation in TranslogEventListener.

Bukhtawar · 2022-06-28T23:44:09Z

I'm trying to glean from this incremental PR if the implementation design is to have 1:1 concrete TranslogManager implementations per translog mechanism. So if a user wants to use Kafka as the Translog we, OpenSearch, would provide a KafkaTranslogManager for handling the kafka specific logic?

The idea is definitely to support streaming stores like kafka and other blob stores to start with. At this point honestly, I am still contemplating if we should introduce a new low level abstraction or continue with TranslogManger as the abstraction itself.

I'll be shortly putting forth a proposal and open up a discussion

/cc @nknize

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar · 2022-06-29T08:49:36Z

Will continue discussion over #3242. Merging it as all feedbacks incorporated

…ct#3671) * Integrate Engine with decoupled translog interface Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Source code changes for integ

9d42ba7

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar requested review from a team and reta as code owners June 23, 2022 19:37

Bukhtawar marked this pull request as draft June 23, 2022 19:37

Bukhtawar changed the title ~~Integrate Engine with decouple Translog interfaces~~ WIP: Integrate Engine with decoupled Translog interfaces Jun 23, 2022

Bukhtawar force-pushed the dc-txlog-2 branch from 23d8488 to 5c3fb6d Compare June 24, 2022 10:01

Test fixes

20772d8

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar force-pushed the dc-txlog-2 branch from 5c3fb6d to 20772d8 Compare June 24, 2022 12:06

opensearch-project deleted a comment from opensearch-ci-bot Jun 24, 2022

Bukhtawar changed the title ~~WIP: Integrate Engine with decoupled Translog interfaces~~ Integrate Engine with decoupled Translog interfaces Jun 24, 2022

Bukhtawar marked this pull request as ready for review June 24, 2022 12:10

Bukhtawar requested a review from nknize June 24, 2022 12:43

opensearch-project deleted a comment from opensearch-ci-bot Jun 24, 2022

Bukhtawar added the v3.0.0 Issues and PRs related to version 3.0.0 label Jun 24, 2022

Bukhtawar commented Jun 24, 2022

View reviewed changes

Test fixes

dc1d0c7

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar force-pushed the dc-txlog-2 branch from a3d45e8 to dc1d0c7 Compare June 24, 2022 14:47

reta reviewed Jun 24, 2022

View reviewed changes

Bukhtawar requested review from reta, mch2 and kartg June 25, 2022 14:30

reta reviewed Jun 27, 2022

View reviewed changes

Review comments

492ffc0

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar requested a review from reta June 28, 2022 13:04

reta approved these changes Jun 28, 2022

View reviewed changes

reta reviewed Jun 28, 2022

View reviewed changes

Merge branch 'main' into dc-txlog-2

ca5619e

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

nknize reviewed Jun 28, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into dc-txlog-2

5f71da7

Bukhtawar requested a review from nknize June 28, 2022 18:35

mch2 approved these changes Jun 28, 2022

View reviewed changes

Merging fail engine

eca2353

Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

Bukhtawar force-pushed the dc-txlog-2 branch from ea2e255 to eca2353 Compare June 29, 2022 06:33

Bukhtawar merged commit 088e019 into opensearch-project:main Jun 29, 2022

imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request Jul 3, 2022

Integrate Engine with decoupled Translog interfaces (opensearch-proje…

cdd9381

…ct#3671) * Integrate Engine with decoupled translog interface Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>

This was referenced Jul 7, 2022

Reverting Engine Changes #3801

Closed

[Backport 2.x] Integrate Engine with decoupled Translog interfaces #3822

Merged

mch2 mentioned this pull request Jul 25, 2022

[Backport 2.x] [Segment Replication] Add a new Engine implementation for replicas with segment replication enabled. #4003

Merged

nknize mentioned this pull request Apr 21, 2023

[BUG] Failure in RemoveCorruptedShardDataCommandTests#testCorruptedTranslog #7268

Open

ashking94 mentioned this pull request Jan 15, 2024

Add missing code in InternalEngine part of #10513 #11883

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Engine with decoupled Translog interfaces #3671

Integrate Engine with decoupled Translog interfaces #3671

Bukhtawar commented Jun 23, 2022 •

edited

Loading

opensearch-ci-bot commented Jun 24, 2022

Bukhtawar Jun 24, 2022

opensearch-ci-bot commented Jun 24, 2022

opensearch-ci-bot commented Jun 24, 2022

reta Jun 24, 2022

Bukhtawar Jun 24, 2022

Bukhtawar Jun 27, 2022

reta Jun 27, 2022

Bukhtawar Jun 27, 2022

reta Jun 27, 2022

Bukhtawar commented Jun 28, 2022

reta Jun 28, 2022

Bukhtawar Jun 28, 2022 •

edited

Loading

Bukhtawar commented Jun 28, 2022

nknize left a comment •

edited

Loading

nknize Jun 28, 2022

nknize Jun 28, 2022

nknize Jun 28, 2022

nknize Jun 28, 2022

Bukhtawar commented Jun 28, 2022 •

edited

Loading

peterzhuamazon commented Jun 28, 2022

nknize commented Jun 28, 2022

mch2 left a comment

mch2 Jun 28, 2022

Bukhtawar commented Jun 28, 2022 •

edited

Loading

Bukhtawar commented Jun 29, 2022

Integrate Engine with decoupled Translog interfaces #3671

Integrate Engine with decoupled Translog interfaces #3671

Conversation

Bukhtawar commented Jun 23, 2022 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented Jun 24, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 24, 2022

opensearch-ci-bot commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar commented Jun 28, 2022

Choose a reason for hiding this comment

Bukhtawar Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Bukhtawar commented Jun 28, 2022

nknize left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar commented Jun 28, 2022 • edited Loading

peterzhuamazon commented Jun 28, 2022

nknize commented Jun 28, 2022

mch2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar commented Jun 28, 2022 • edited Loading

Bukhtawar commented Jun 29, 2022

Bukhtawar commented Jun 23, 2022 •

edited

Loading

Bukhtawar Jun 28, 2022 •

edited

Loading

nknize left a comment •

edited

Loading

Bukhtawar commented Jun 28, 2022 •

edited

Loading

Bukhtawar commented Jun 28, 2022 •

edited

Loading