Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Segment Replication] Add PIT/Scroll compatibility with Segment Replication #6644 #6765

Merged
merged 8 commits into from
Apr 5, 2023

Conversation

dreamer-89
Copy link
Member

@dreamer-89 dreamer-89 commented Mar 20, 2023

Taken from #6644


Description

This change makes updates to make PIT/Scroll queries compatible with Segment Replication. It does this by refcounting files when a new reader is created, and discarding those files after a reader is closed.

Issues Resolved

#6519

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
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member Author

Gradle Check (Jenkins) Run Completed with:

Failure tracked in #5329

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation" -Dtests.seed=2ABB5BB826EE60DE -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-NI -Dtests.timezone=Asia/Colombo -Druntime.java=19

org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests > testTaskResourceTrackingDuringTaskCancellation FAILED
    java.lang.AssertionError: expected:<0> but was:<1>
        at __randomizedtesting.SeedInfo.seed([2ABB5BB826EE60DE:F02BEE75FC0EDA5F]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:633)
        at org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation(ResourceAwareTasksTests.java:392)

@anasalkouz anasalkouz linked an issue Mar 22, 2023 that may be closed by this pull request
Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I threw in a few nitpicks but overall a nice solution and LGTM. Let's also javadoc at least the critical path logic along with the test workflow. I threw in some suggested docs you can change, but probably worth adding a bit more explanation for anyone to be able to track the thought process.

@dreamer-89
Copy link
Member Author

dreamer-89 commented Mar 29, 2023

@nknize @mch2 : Updated PR. Please have a look.

Changes:

  1. Review comments related updated
  2. Prevent temporary file deletion while closing OpenSearchReaderManager. Without this change, temporary files were getting deleted on reader close resulting in segment replication failure, followed by replica shard failure. Added integration test to validate it. Test failure stack trace below(thanks to @mch2 for pointing this issue).
[2023-03-30T01:12:26,164][ERROR][o.o.i.r.SegmentReplicationTargetService] [node_t1] [Thread 44] replication failure
org.opensearch.indices.replication.common.ReplicationFailedException: [test-idx-1][0]: Replication failed on  (failed to clean after replication)
...
Caused by: java.nio.file.NoSuchFileException: /private/var/folders/6b/gsf85p0j4w1bgzhr_hlvbzcr0000gs/T/org.opensearch.indices.replication.SegmentReplicationIT_CB26F97FEEB82886-001/tempDir-002/node_t1/nodes/0/indices/3BmeL0kQQU6ZRsrUpTmIYg/0/index/replication.8ODgn4KDR-yjblfW8Rtybg._1.cfe -> /private/var/folders/6b/gsf85p0j4w1bgzhr_hlvbzcr0000gs/T/org.opensearch.indices.replication.SegmentReplicationIT_CB26F97FEEB82886-001/tempDir-002/node_t1/nodes/0/indices/3BmeL0kQQU6ZRsrUpTmIYg/0/index/_1.cfe
...
  1. Use NRTReplicationEngine.lastCommittedSegmentInfos instead of reading SegmentInfos from disk. Coming from discussion here
  2. Prevent inc'refing segment info file (segments_N) which causes concurrency issue when OpenSearchReaderManager receives the segment infos from primary with segments_N file linked to older commit files (this needs to looked separately). Reader then incorrectly inc'ref segments_N-1 file with currently copied segment files. This segments_N-1 is then prevented from deletion but cause issues while listing commits. Logs on primary depicts this
[2023-04-05T00:25:50,242][INFO ][o.o.i.r.c.CopyState      ] [node_t0] [Thread 46] --> Sending segmentInfos [_d_Lucene90_0.pos, _d_Lucene90_0.dvd, _7.cfs, _d.fdx, _d_Lucene90_0.tim, _a.si, _d_Lucene90_0.tmd, _d.fdm, _d.kdi, _7.cfe, _d.kdd, _d.si, _7.si, _d.fdt, _d_Lucene90_0.tip, _d_Lucene90_0.dvm, _d.kdm, _a.cfe, _d_Lucene90_0.doc, _4.cfe, _d.nvd, _d.nvm, _d.fnm, _a.cfs, segments_4, _4.cfs, _4.si]
[2023-04-05T00:25:50,246][INFO ][o.o.i.r.c.CopyState      ] [node_t0] [Thread 46] --> On disk committed SegmentInfos [_0.cfe, _0.si, _0.cfs, segments_4]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.SearchableSnapshotIT.testCacheFilesAreClosedAfterUse
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.shrink/30_copy_settings/Copy settings during shrink index}

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member Author

dreamer-89 commented Mar 31, 2023

The existing implementation results in occasional failure of testRelocateWhileContinuouslyIndexingAndWaitingForRefresh (also cause of last two gradle check failrues). The test times out while waiting for cluster health response post primary shard relocation. The timeout happens during engine reset while reading all IndexCommits from directory. The thread simply hangs without ever returning. It appears that files incRef'ed inside OpenSearchReaderManager fiddles with follow up (reset engine workflow) DirectoryReader.listCommits call. @nknize : Would you know cause of this flaky failure ?

This flaky failure is blocking merge of this PR

@dreamer-89
Copy link
Member Author

I observed that test hangs indefinitely while listing all IndexCommits only when there are non-compressed segment files (.fdt, .fdx, .fnm etc) on store. When compound files (only .cfs, .cfe) are used, the test passes.

Store content & files in latest commits

For failing test

[2023-04-03T05:04:18,196][INFO ][o.o.c.l.Lucene           ] [node_t1] [Thread 71] --> Store files [_7.cfe, _7.cfs, _7.si, _8.cfe, _8.cfs, _8.si, _a.cfe, _a.cfs, _a.si, _d.fdm, _d.fdt, _d.fdx, _d.fnm, _d.kdd, _d.kdi, _d.kdm, _d.nvd, _d.nvm, _d.si, _d_Lucene90_0.doc, _d_Lucene90_0.dvd, _d_Lucene90_0.dvm, _d_Lucene90_0.pos, _d_Lucene90_0.tim, _d_Lucene90_0.tip, _d_Lucene90_0.tmd, segments_4, segments_5, write.lock]
[2023-04-03T05:04:18,205][INFO ][o.o.c.l.Lucene           ] [node_t1] [Thread 71] --> Files in latest SegmentInfos [_d_Lucene90_0.pos, _d_Lucene90_0.dvd, _7.cfs, _d.fdx, _d_Lucene90_0.tim, _a.si, _d_Lucene90_0.tmd, _d.fdm, _d.kdi, _7.cfe, _d.kdd, _d.si, _7.si, _d.fdt, _d_Lucene90_0.tip, _d_Lucene90_0.dvm, _d.kdm, _a.cfe, _d_Lucene90_0.doc, _d.nvd, _d.nvm, _d.fnm, _8.cfe, segments_5, _a.cfs, _8.cfs, _8.si]

Passing test

[2023-04-03T05:06:23,580][INFO ][o.o.c.l.Lucene           ] [node_t1] [Thread 216] --> Store files [_0.cfe, _0.cfs, _0.si, _1.cfe, _1.cfs, _1.si, _2.cfe, _2.cfs, _2.si, _3.cfe, _3.cfs, _3.si, _4.cfe, _4.cfs, _4.si, _5.cfe, _5.cfs, _5.si, _6.cfe, _6.cfs, _6.si, segments_4, segments_5, write.lock]
[2023-04-03T05:06:23,588][INFO ][o.o.c.l.Lucene           ] [node_t1] [Thread 216] --> Files in latest SegmentInfos [_6.cfe, _0.si, _3.si, _2.si, _1.cfe, _1.si, _0.cfs, _5.cfs, _2.cfs, _4.cfe, _3.cfe, segments_5, _1.cfs, _0.cfe, _6.cfs, _2.cfe, _3.cfs, _5.cfe, _4.cfs, _4.si, _5.si, _6.si]

For debugging purpose, I tried to move the DirectoryReader.listCommits(directory) code inside OpenSearch but it uses package private utilities that prevents re-use.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.indices.replication.SegmentReplicationIT.testScrollWithOngoingSegmentReplication
      1 org.opensearch.index.store.remote.utils.TransferManagerTests.testConcurrentAccess

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #6765 (1f057c4) into main (95c6ed9) will decrease coverage by 0.02%.
The diff coverage is 68.10%.

📣 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    #6765      +/-   ##
============================================
- Coverage     70.78%   70.76%   -0.02%     
+ Complexity    59305    59296       -9     
============================================
  Files          4813     4824      +11     
  Lines        283781   284055     +274     
  Branches      40924    40955      +31     
============================================
+ Hits         200864   201007     +143     
- Misses        66420    66562     +142     
+ Partials      16497    16486      -11     
Impacted Files Coverage Δ
...ch/index/codec/customcodecs/CustomCodecPlugin.java 0.00% <0.00%> (ø)
...h/index/codec/customcodecs/CustomCodecService.java 0.00% <0.00%> (ø)
.../codec/customcodecs/CustomCodecServiceFactory.java 0.00% <0.00%> (ø)
...ustomcodecs/PerFieldMappingPostingFormatCodec.java 0.00% <0.00%> (ø)
...customcodecs/Lucene95CustomStoredFieldsFormat.java 25.00% <25.00%> (ø)
.../index/codec/customcodecs/Lucene95CustomCodec.java 60.00% <60.00%> (ø)
...rc/main/java/org/opensearch/index/store/Store.java 79.37% <61.53%> (-0.26%) ⬇️
.../codec/customcodecs/ZstdNoDictCompressionMode.java 76.71% <76.71%> (ø)
...opensearch/index/codec/customcodecs/ZstdCodec.java 80.00% <80.00%> (ø)
...arch/index/codec/customcodecs/ZstdNoDictCodec.java 80.00% <80.00%> (ø)
... and 5 more

... and 485 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.

mch2 and others added 7 commits April 4, 2023 11:11
This change makes updates to make PIT/Scroll queries compatibile with Segment Replication.
It does this by refcounting files when a new reader is created, and discarding those files after
a reader is closed.

Signed-off-by: Marc Handalian <handalm@amazon.com>
Signed-off-by: Marc Handalian <handalm@amazon.com>
…of current store state.

Signed-off-by: Marc Handalian <handalm@amazon.com>
…close

Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Suraj Singh <surajrider@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

@dreamer-89 dreamer-89 merged commit 4b7cb02 into opensearch-project:main Apr 5, 2023
@dreamer-89 dreamer-89 added the backport 2.x Backport to 2.x branch label Apr 5, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

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
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6765-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4b7cb022d548f976911a55c58a37a758620b2ef6
# Push it to GitHub
git push --set-upstream origin backport/backport-6765-to-2.x
# Go back to the original working tree
popd
# 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-6765-to-2.x.

dreamer-89 added a commit to dreamer-89/OpenSearch that referenced this pull request Apr 5, 2023
…cation opensearch-project#6644 (opensearch-project#6765)

* Segment Replication - PIT/Scroll compatibility.

This change makes updates to make PIT/Scroll queries compatibile with Segment Replication.
It does this by refcounting files when a new reader is created, and discarding those files after
a reader is closed.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Fix broken test.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Fix test bug with PIT where snapshotted segments are queried instead of current store state.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Address review comments and prevent temp file deletion during reader close

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Fix precommit failure

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Use last committed segment infos reference from replication engine

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Clean up and prevent incref on segment info file copied from primary

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Fix failing test

Signed-off-by: Suraj Singh <surajrider@gmail.com>

---------

Signed-off-by: Marc Handalian <handalm@amazon.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Co-authored-by: Marc Handalian <handalm@amazon.com>
dreamer-89 added a commit that referenced this pull request Apr 5, 2023
…cation (#7010)

* [Segment Replication] Add PIT/Scroll compatibility with Segment Replication #6644 (#6765)

* Segment Replication - PIT/Scroll compatibility.

This change makes updates to make PIT/Scroll queries compatibile with Segment Replication.
It does this by refcounting files when a new reader is created, and discarding those files after
a reader is closed.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Fix broken test.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Fix test bug with PIT where snapshotted segments are queried instead of current store state.

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Address review comments and prevent temp file deletion during reader close

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Fix precommit failure

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Use last committed segment infos reference from replication engine

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Clean up and prevent incref on segment info file copied from primary

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Fix failing test

Signed-off-by: Suraj Singh <surajrider@gmail.com>

---------

Signed-off-by: Marc Handalian <handalm@amazon.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Co-authored-by: Marc Handalian <handalm@amazon.com>

* Add param definition causing precommit failure

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Remove unnecessary override annotation

Signed-off-by: Suraj Singh <surajrider@gmail.com>

---------

Signed-off-by: Marc Handalian <handalm@amazon.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Co-authored-by: Marc Handalian <handalm@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Segment replication] Point in time search comaptibility
5 participants