Skip to content

Fix handling of deletes during leading segment merge#242

Merged
akash-shankaran merged 6 commits into
opensearch-project:mainfrom
ashkrisk:fix-lsm-deletes
Feb 11, 2026
Merged

Fix handling of deletes during leading segment merge#242
akash-shankaran merged 6 commits into
opensearch-project:mainfrom
ashkrisk:fix-lsm-deletes

Conversation

@ashkrisk

@ashkrisk ashkrisk commented Jan 30, 2026

Copy link
Copy Markdown
Contributor

Description

This PR fixes the issues with the leading segment merge logic which show up when delete operations are involved. A set of test cases are added which fail in the absence of the changes in this PR (prior to the revert).

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@akash-shankaran
akash-shankaran requested a review from reta January 30, 2026 20:08
@ashkrisk
ashkrisk force-pushed the fix-lsm-deletes branch 2 times, most recently from 1a86b39 to ef6ffa2 Compare February 2, 2026 10:59
@ashkrisk

ashkrisk commented Feb 2, 2026

Copy link
Copy Markdown
Contributor Author

I'm mostly done with the main code, barring a few tweaks. I'm still planning to add better tests.

@ashkrisk

ashkrisk commented Feb 3, 2026

Copy link
Copy Markdown
Contributor Author

This PR is essentially done, but there are conflicts because of the revert that was just merged.

@akash-shankaran

Copy link
Copy Markdown
Collaborator

This PR is essentially done, but there are conflicts because of the revert that was just merged.

Hey @ashkrisk I did mention this to Ted yesterday. If you need to revive the change in your PR, please feel free to do so. Let us know when we can review this.

…ch-project#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
@ashkrisk

ashkrisk commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author
  • The first commit in this PR is an "undo revert" commit, reverting Revert of support deletes for incremental insertion #240.
  • The second commit is the actual fix and test.
  • The third commit removes some of the unnecessary changes in the "undo revert" so that the splash zone is lowered, hopefully making it easier to review as a whole.

@ashkrisk
ashkrisk marked this pull request as ready for review February 5, 2026 10:27
Comment thread src/main/java/org/opensearch/knn/index/codec/jvector/JVectorWriter.java Outdated
@akash-shankaran

Copy link
Copy Markdown
Collaborator

how do we disable this feature in production if we had to? Is it possible to enable/disable it via a config?

This is to avoid a repeat of what happened in our scale test environment.

@ashkrisk

ashkrisk commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Leading segment merge is automatically disabled once the number of live vectors involved the merge crosses the threshold for using PQ. One way to turn it off completely is to set a PQ threshold below the minimum segment size.

Note that the error you saw in the test environment wasn't because of leading segment merge per se, rather because of a bug in the way that ord <-> docId mappings were being saved. The bug was introduced as part of the changes to support deletions in the context of leading segment merge.

@akash-shankaran

Copy link
Copy Markdown
Collaborator

Note that the error you saw in the test environment wasn't because of leading segment merge per se, rather because of a bug in the way that ord <-> docId mappings were being saved. The bug was introduced as part of the changes to support deletions in the context of leading segment merge.

Thanks for alleviating my concern here. Yes, understand the bug was due to the ordinal mappings, which your PR addresses.
My concern is with the deletes path running into any unforeseen issues in production, and us having no way to disable the feature (i.e. incremental graph build during deletes). If there is a way to safely/enable this feature via a config (fall back to full graph rebuild), that would be great. We can remove the config after a couple of releases, when no issues are reported.
Please also let me know if you think this is unnecessary, and won't happen after your fix.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Also tweak some logs

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
@ashkrisk

ashkrisk commented Feb 9, 2026

Copy link
Copy Markdown
Contributor Author

I added a configuration parameter advanced.leading_segment_merge_disabled which can be set to true to skip leading segment merge entirely.

Note that I added this by looking at how the other parameters were configured. It should be enough to allow the configuration to be set in Opensearch and propagated to the plugin, and the test in JVectorEngineIT seems to recognize it, but let me know if that's not the case.

@akash-shankaran
akash-shankaran merged commit f33bb41 into opensearch-project:main Feb 11, 2026
22 of 33 checks passed
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Feb 11, 2026
* Revert "Revert of support deletes for incremental insertion (#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Fix leading segment merge under deletions

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add/fix changelog entries

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Review comment updates

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add config to disable leading segment merge

Also tweak some logs

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

---------

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
(cherry picked from commit f33bb41)
@opensearch-trigger-bot

Copy link
Copy Markdown
Contributor

The backport to 3.3 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-3.3 3.3
# Navigate to the new working tree
cd .worktrees/backport-3.3
# Create a new branch
git switch --create backport/backport-242-to-3.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 f33bb417ae2a63d5902426bbf0f8d7aa07d80e7d
# Push it to GitHub
git push --set-upstream origin backport/backport-242-to-3.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-3.3

Then, create a pull request where the base branch is 3.3 and the compare/head branch is backport/backport-242-to-3.3.

opensearch-trigger-bot Bot pushed a commit that referenced this pull request Feb 11, 2026
* Revert "Revert of support deletes for incremental insertion (#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Fix leading segment merge under deletions

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add/fix changelog entries

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Review comment updates

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add config to disable leading segment merge

Also tweak some logs

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

---------

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
(cherry picked from commit f33bb41)
akash-shankaran pushed a commit that referenced this pull request Feb 11, 2026
* Revert "Revert of support deletes for incremental insertion (#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.



* Fix leading segment merge under deletions



* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.



* Add/fix changelog entries



* Review comment updates



* Add config to disable leading segment merge

Also tweak some logs



---------


(cherry picked from commit f33bb41)

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Co-authored-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
reta pushed a commit to reta/opensearch-jvector that referenced this pull request Feb 11, 2026
…ect#242)

* Revert "Revert of support deletes for incremental insertion (opensearch-project#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Fix leading segment merge under deletions

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add/fix changelog entries

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Review comment updates

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

* Add config to disable leading segment merge

Also tweak some logs

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>

---------

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
(cherry picked from commit f33bb41)
akash-shankaran added a commit that referenced this pull request Feb 11, 2026
* Revert "Revert of support deletes for incremental insertion (#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.



* Fix leading segment merge under deletions



* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.



* Add/fix changelog entries



* Review comment updates



* Add config to disable leading segment merge

Also tweak some logs



---------


(cherry picked from commit f33bb41)

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Co-authored-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Co-authored-by: Akash Shankaran <akash.shankaran1@gmail.com>
akash-shankaran pushed a commit that referenced this pull request Feb 11, 2026
* Revert "Revert of support deletes for incremental insertion (#240)"

This reverts commit 4453ba9.

Reverting the revert to be able to apply the fix.



* Fix leading segment merge under deletions



* Refine the "undo revert" commit

These commits will all be squashed anyway, so this change helps reduce
the splash zone of the final squashed commit without affecting
functionality.



* Add/fix changelog entries



* Review comment updates



* Add config to disable leading segment merge

Also tweak some logs



---------


(cherry picked from commit f33bb41)

Signed-off-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Co-authored-by: Ashwin Krishna Kumar <nebulousmagneticwind@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants