Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implements delete datasource API #291
Implements delete datasource API #291
Changes from all commits
766f760
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not a write lock that can be grabbed to delete the datasource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't implemented lock between create ip2geo processor and delete datasource.
Appreciate if you could share some idea on the write lock. If write lock involves interacting with index, it cannot be used during ip2geo processor creation phase because processor creation is a process which changes cluster state and while changing cluster state, we cannot call some or all of OpenSearch API like getting or searching document from an index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm yes Im not sure what the lock would be. I would think there would be something with the JobScheduler LockService.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lock with job scheduler utilize opensearch index which cannot be used in processor creation time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like utilizing cluster state might be the best way to avoid this.
In k-NN, we do something similar with the ModelGraveyard: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/indices/ModelGraveyard.java. Basically, we add model IDs that are being deleted to a "ModelGraveyard" that is stored in the cluster state.
ref: opensearch-project/k-NN#424. @naveentatikonda might be a good person to talk to more about this - he implemented it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The thing is, we didn't want to add any custom metadata during design review meeting as it can cause a headache in the future due to many restriction on maintaining backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, might be something to consider in the future if this becomes a problem.