Always use Lucene index in peer recovery #2077

nknize · 2022-02-09T14:48:52Z

With soft deletes no longer optional, this PR switches peer recovery to always use the
lucene index instead of replaying operations from the translog. This reduces disk footprint
and storage costs for the end user by relying on improved stored fields compression. Note
there is a slight CPU performance penalty that has recently been improved in recent releases
of lucene. For now we accept the CPU performance penalty in trade for smaller disk footprint
and storage cost. If desired we can explore offering both options that are selectable by an
expert setting (similar to index.codec).

With soft deletes no longer optional, peer recovery is switched to always use the lucene index instead of replaying operations from the translog. Signed-off-by: Nicholas Walter Knize <[email protected]>

opensearch-ci-bot · 2022-02-09T14:50:02Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-02-09T15:35:19Z

✅ Gradle Check success 55881a1
Log 2307

Reports 2307

nknize · 2022-02-22T20:28:39Z

I'm getting ready to remove types from the translog....having this PR merged will help prevent me from having to carry some unnecessary code to handle types in recovery from translog.

Can someone please review / approve to help get this in relatively soon? /cc @CEHENKLE @andrross

reta · 2022-02-22T21:45:54Z

server/src/main/java/org/opensearch/indices/recovery/RecoverySourceHandler.java

            resources.add(retentionLock);
            final long startingSeqNo;
            final boolean isSequenceNumberBasedRecovery = request.startingSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO
                && isTargetSameHistory()
-                && shard.hasCompleteHistoryOperations("peer-recovery", historySource, request.startingSeqNo())
-                && (historySource == Engine.HistorySource.TRANSLOG
+                && shard.hasCompleteHistoryOperations("peer-recovery", request.startingSeqNo())


Very difficult to follow this ifs forest, may be a few intermediate booleans would help?

I think I'd rather do that in a follow up because some of this is going to get refactored away anyway with coming segrep changes

server/src/main/java/org/opensearch/indices/recovery/RecoverySourceHandler.java

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

Signed-off-by: Nicholas Walter Knize <[email protected]>

opensearch-ci-bot · 2022-02-23T06:20:58Z

✅ Gradle Check success 6f20e72
Log 2735

Reports 2735

andrross · 2022-02-23T16:01:54Z

server/src/main/java/org/opensearch/indices/recovery/RecoverySourceHandler.java

-                    shard.estimateNumberOfHistoryOperations("peer-recovery", historySource, startingSeqNo)
-                );
-                final Translog.Snapshot phase2Snapshot = shard.getHistoryOperations("peer-recovery", historySource, startingSeqNo);
+                logger.trace("snapshot translog for recovery; current size is [{}]", estimateNumberOfHistoryOperations(startingSeqNo));


This isn't directly related to the change here, but would it be better to refactor this logging statement so that estimateNumberOfHistoryOperations() only gets computed if trace logging is actually enabled? It looks like that method is doing a non-trivial amount of work. I don't know the context in which this is called to know if it would make any practical difference though.

💯 Good catch! Wrapped in logger.isTraceEnabled()

Signed-off-by: Nicholas Walter Knize <[email protected]>

opensearch-ci-bot · 2022-02-23T17:02:35Z

✅ Gradle Check success 8a21554
Log 2747

Reports 2747

saratvemulapalli · 2022-03-17T17:12:30Z

@nknize / @andrross / @reta looks like we have removed getHistoryOperations.
Is there an alternative way to get the history on index ?
Looks like #2482 CCR plugin depends on it to understand the changes and replicate.

reta · 2022-03-17T17:15:41Z

@nknize / @andrross / @reta looks like we have removed getHistoryOperations.

@saratvemulapalli hm ... it is there but signature has changed a bit:

  public Translog.Snapshot getHistoryOperations(String reason, long startingSeqNo, long endSeqNo) throws IOException {

nknize · 2022-03-17T17:20:53Z

Correct. Just add true for accurate counts.

Note that I almost removed this because it isn't used anywhere in core. I kept it around in case plugins (e.g., CCR) need it. We might consider refactoring to the CCR plugin if it's the only one that uses it, or we can refactor to common-utils module if we need to reuse across plugins.

saratvemulapalli · 2022-03-17T17:24:44Z

@nknize / @andrross / @reta looks like we have removed getHistoryOperations.

@saratvemulapalli hm ... it is there but signature has changed a bit:
  public Translog.Snapshot getHistoryOperations(String reason, long startingSeqNo, long endSeqNo) throws IOException {

Sorry I should have been more specific. CCR was depending on getHistoryOperations based on Engine.HistorySource.

nknize · 2022-03-17T17:29:45Z

@nknize / @andrross / @reta looks like we have removed getHistoryOperations.

@saratvemulapalli hm ... it is there but signature has changed a bit:
  public Translog.Snapshot getHistoryOperations(String reason, long startingSeqNo, long endSeqNo) throws IOException {
Sorry I should have been more specific. CCR was depending on getHistoryOperations based on Engine.HistorySource.

Since soft deletes are no longer optional, Translog is also no longer an option for HistorySource. We now always use the Lucene Index per this PR.

saratvemulapalli · 2022-03-17T17:47:02Z

@nknize / @andrross / @reta looks like we have removed getHistoryOperations.

@saratvemulapalli hm ... it is there but signature has changed a bit:
  public Translog.Snapshot getHistoryOperations(String reason, long startingSeqNo, long endSeqNo) throws IOException {
Sorry I should have been more specific. CCR was depending on getHistoryOperations based on Engine.HistorySource.
Since soft deletes are no longer optional, Translog is also no longer an option for HistorySource. We now always use the Lucene Index per:

#2077

So if I understand this correctly, the only way for plugins to pull history is get the information from lucene index (a.k.a seqNos).
If that assumption is correct, then we are saying CCR has to change to use the new signature and not depend on translog anymore.

nknize · 2022-03-17T18:50:21Z

CCR has to change to use the new signature and not depend on translog anymore.

CCR has to change to use the new signature, yes, and not depend on the "translog" file as a history source. CCR still gets operations history but it's now from the lucene operations index instead of the translog file.

Always use Lucene index in peer recovery

55881a1

With soft deletes no longer optional, peer recovery is switched to always use the lucene index instead of replaying operations from the translog. Signed-off-by: Nicholas Walter Knize <[email protected]>

nknize added enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 Storage:Durability Issues and PRs related to the durability framework labels Feb 9, 2022

nknize requested a review from a team as a code owner February 9, 2022 14:48

dblock requested review from andrross and mch2 February 9, 2022 16:30

reta reviewed Feb 22, 2022

View reviewed changes

server/src/main/java/org/opensearch/indices/recovery/RecoverySourceHandler.java Outdated Show resolved Hide resolved

reta reviewed Feb 22, 2022

View reviewed changes

server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java Show resolved Hide resolved

changes from PR feedback

6f20e72

Signed-off-by: Nicholas Walter Knize <[email protected]>

reta approved these changes Feb 23, 2022

View reviewed changes

andrross approved these changes Feb 23, 2022

View reviewed changes

wrap trace log in tracelogger.enabled

8a21554

Signed-off-by: Nicholas Walter Knize <[email protected]>

nknize merged commit 44441d8 into opensearch-project:main Feb 23, 2022

nknize mentioned this pull request Mar 12, 2022

[Remove] types from translog #2439

Merged

saikaranam-amazon mentioned this pull request Mar 16, 2022

[BUG] Breaking changes for CCR plugin in OS 2.0 #2482

Closed

nknize mentioned this pull request Apr 12, 2022

[RFC] Cross Cluster Replication as a Core Component #2872

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use Lucene index in peer recovery #2077

Always use Lucene index in peer recovery #2077

nknize commented Feb 9, 2022 •

edited

Loading

opensearch-ci-bot commented Feb 9, 2022

opensearch-ci-bot commented Feb 9, 2022

nknize commented Feb 22, 2022

reta Feb 22, 2022

nknize Feb 23, 2022

opensearch-ci-bot commented Feb 23, 2022

andrross Feb 23, 2022

nknize Feb 23, 2022

opensearch-ci-bot commented Feb 23, 2022

saratvemulapalli commented Mar 17, 2022

reta commented Mar 17, 2022

nknize commented Mar 17, 2022

saratvemulapalli commented Mar 17, 2022

nknize commented Mar 17, 2022 •

edited

Loading

saratvemulapalli commented Mar 17, 2022

nknize commented Mar 17, 2022

Always use Lucene index in peer recovery #2077

Always use Lucene index in peer recovery #2077

Conversation

nknize commented Feb 9, 2022 • edited Loading

opensearch-ci-bot commented Feb 9, 2022

opensearch-ci-bot commented Feb 9, 2022

nknize commented Feb 22, 2022

reta Feb 22, 2022

Choose a reason for hiding this comment

nknize Feb 23, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Feb 23, 2022

andrross Feb 23, 2022

Choose a reason for hiding this comment

nknize Feb 23, 2022

Choose a reason for hiding this comment

opensearch-ci-bot commented Feb 23, 2022

saratvemulapalli commented Mar 17, 2022

reta commented Mar 17, 2022

nknize commented Mar 17, 2022

saratvemulapalli commented Mar 17, 2022

nknize commented Mar 17, 2022 • edited Loading

saratvemulapalli commented Mar 17, 2022

nknize commented Mar 17, 2022

nknize commented Feb 9, 2022 •

edited

Loading

nknize commented Mar 17, 2022 •

edited

Loading