-
Notifications
You must be signed in to change notification settings - Fork 114
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
[Keyword search] Remove node from index on delete #9520
Conversation
2dd2380
to
5f334e9
Compare
Description --- Fixes #9460 Also moves without changing search_store pre-existing functions Risks --- - slow down or fail deletes => unlikely given speed of deletion - miss some nodes deletion Deploy --- core
5f334e9
to
c2a1485
Compare
@@ -82,54 +87,6 @@ const NODES_INDEX_NAME: &str = "core.data_sources_nodes"; | |||
|
|||
#[async_trait] | |||
impl SearchStore for ElasticsearchSearchStore { | |||
async fn index_document(&self, document: Document) -> Result<()> { |
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.
here it's just moved around, apart from the 2 new delete functions
pub async fn delete( | ||
&self, | ||
store: Box<dyn Store + Sync + Send>, | ||
databases_store: Box<dyn DatabasesStore + Sync + Send>, | ||
search_store: Option<Box<dyn SearchStore + Sync + Send>>, |
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.
why optional?
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.
When deleting a datasource, there's a batch delete on all nodes on a given datasource id, fastest way to do it
I thought in that case it was costly and useless to delete tables one by one (ES average delete is likely ~5-10ms)
core/bin/core_api.rs
Outdated
})), | ||
}), | ||
), | ||
Ok(_) => match state.search_store.delete_node(folder_id).await { |
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.
Smells like we want a Folder object that handles the delete logic :)
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.
to be consistent with the absence of consitency (since document does not do delete logic via an object :) ) I made the simplest most straightforward code :)
but happy to change
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.
The succession of api error handling is a bit distasteful here. Won't block on it of course
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.
true, refactored to avoid that
Description
Fixes #9460
Also moves without changing search_store pre-existing functions
Risks
Deploy
core