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

Fixed LeafReaders casting errors to SegmentReaders when segment repli… #1808

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

navneet1v
Copy link
Collaborator

@navneet1v navneet1v commented Jul 10, 2024

Description

Fixed LeafReaders casting errors to SegmentReaders when segment replication is enabled during search

Root Cause

When Segment replication is enabled, then for replicas IndexReaders are opened via passing directory path, rather than using IndexWriters to Open the IndexReaders. This happens because replicas don't take write for Segment Replication. To ensure that soft deletes are applied on replicas too, seg-rep engine uses SoftDeletesDirectoryReaderWrapper(which extends the FilterDirectoryReader) to open the directory with soft deletes field. During search when the LeafReader for this directory is provided the SegmentReader gets wrapped into different readers like SoftDeletesFilterCodecReader or SoftDeletesFilterLeafReader. So in order to get SegmentReader we need unwrap the reader properly. Here, the idea is first we unwrap the Reader via FilterLeafReader and if the instance is of FilterCodecReader(base class for SoftDeletesFilterCodecReader), we need another round of unwrapping via FilterCodecReader to get SegmentReader. If we don't do this kind of unwrapping then during query we get cast exceptions on query on replicas with deletes in segment replication cases. We are using a helper function from OpenSearch Core to do the same.

Future Improvement:

  1. May be we can create 1 more GH action which is special for multi node testing and just have some annotations on IT to say whether this test should run under multi node cluster or not. Will see what options are available for that. Currently I have tested locally using
./gradlew integTest --tests "org.opensearch.knn.index.SegmentReplicationIT.testSearchOnReplicas_whenIndexHasDeletedDocs_thenSuccess" -PnumNodes=2

Issues Resolved

#1807

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as 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.

@navneet1v
Copy link
Collaborator Author

The BWC test will be fixed by this PR: #1811

…cation is enabled during search

Signed-off-by: Navneet Verma <[email protected]>
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

Copy link
Member

@vibrantvarun vibrantvarun left a comment

Choose a reason for hiding this comment

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

LGTM

@navneet1v navneet1v added the Bug Fixes Changes to a system or product designed to handle a programming bug/glitch label Jul 16, 2024
@navneet1v navneet1v merged commit 62cdbb6 into opensearch-project:main Jul 16, 2024
54 of 55 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 16, 2024
…cation is enabled during search (#1808)

Signed-off-by: Navneet Verma <[email protected]>
(cherry picked from commit 62cdbb6)
navneet1v added a commit that referenced this pull request Jul 16, 2024
…cation is enabled during search (#1808)

Signed-off-by: Navneet Verma <[email protected]>
ryanbogan pushed a commit to ryanbogan/k-NN that referenced this pull request Jul 16, 2024
navneet1v added a commit that referenced this pull request Jul 16, 2024
…cation is enabled during search (#1808) (#1836)

Signed-off-by: Navneet Verma <[email protected]>
Co-authored-by: Navneet Verma <[email protected]>
naveentatikonda pushed a commit to naveentatikonda/k-NN that referenced this pull request Jul 20, 2024
…cation is enabled during search (opensearch-project#1808) (opensearch-project#1836)

Signed-off-by: Navneet Verma <[email protected]>
Co-authored-by: Navneet Verma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Bug Fixes Changes to a system or product designed to handle a programming bug/glitch
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants