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.

Sign up for GitHub

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

Jump to bottom

Add rolling version upgrade support for ignore_unmapped #1770

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add rolling version upgrade support for ignore_unmapped #1770

wants to merge 3 commits into from

Conversation

kotwanikunal
Copy link
Member

@kotwanikunal kotwanikunal commented Jun 25, 2024 •
edited by navneet1v
Loading

Description

  • Adds in rolling version upgrade support for ignore_unmapped

Issues Resolved

  • N/A

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.

@kotwanikunal kotwanikunal mentioned this pull request Jun 25, 2024
Open
@navneet1v
Copy link
Collaborator

@kotwanikunal thanks for raising the PR. Will wait for all BWC to gh actions to complete.

@navneet1v
Copy link
Collaborator

@kotwanikunal can you fix the failed GH actions?

@martin-gaievski
Copy link
Member

Good finding, we should do same in other repos. Will approve once CI is fixed. Current error is from spotless, you can run spotlessApply

@kotwanikunal
Copy link
Member Author

Pushed linting fixes in.

@kotwanikunal
Copy link
Member Author

Looks related to the change. Let me take a look

> Task :test

Tests with failures:
 - org.opensearch.knn.index.query.KNNQueryBuilderTests.testSerialization

shatejas
shatejas reviewed Jul 3, 2024
View reviewed changes
src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java Outdated
@@ -324,7 +326,7 @@ protected void doWriteTo(StreamOutput out) throws IOException {
out.writeFloatArray(vector);
out.writeInt(k);
out.writeOptionalNamedWriteable(filter);
if (isClusterOnOrAfterMinRequiredVersion("ignore_unmapped")) {
if (out.getVersion().onOrAfter(getMinVersionForFeature("ignore_unmapped"))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this stream guarantee the min version of the cluster. Just verifying since the previous method has that check

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a historical reason that we look for the cluster version and not the stream version?
It should behave the same - a feature is on only when both the nodes are upgraded.

@jmazanec15 / @navneet1v

Copy link
Collaborator

Choose a reason for hiding this comment

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

We haven't serialized the stream version when we write KNNQueryBuilder at first. Therefore, we were not able to use stream version. After that, we haven't tried to add stream version because cluster version just worked fine for us.

@jmazanec15
Copy link
Member

@kotwanikunal is there pending?

@kotwanikunal kotwanikunal force-pushed the rolling-version-support branch from 7550f9e to 3aa15a2 Compare October 29, 2024 23:05
@kotwanikunal
Copy link
Member Author

@kotwanikunal is there pending?

I had a rebase pending.
Updated the PR with latest references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shatejas shatejas shatejas left review comments

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kotwanikunal @navneet1v @martin-gaievski @jmazanec15 @heemin32 @shatejas