Skip to content

perf: Faster index::update_index IT tests #5579

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

Merged

Conversation

martin-g
Copy link
Contributor

Pull Request

Related issue

#4840

What does this PR do?

  • Use a shared server where possible.
  • Assert succeeded/failed task waits.

Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Hey, thanks for tackling all these issues!

Comment on lines 80 to 94
let index = server.unique_index();
let (create_task, code) = index.create(Some("id")).await;

assert_eq!(code, 202);
index.wait_task(create_task.uid()).await.succeeded();

let documents = json!([
{
"id": "11",
"content": "foobar"
}
]);
index.add_documents(documents, None).await;
let (add_docs_task, add_docs_status_code) = index.add_documents(documents, None).await;
assert_eq!(add_docs_status_code, 202);
index.wait_task(add_docs_task.uid()).await.succeeded();
Copy link
Member

Choose a reason for hiding this comment

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

In this case instead of creating a now index and pushing documents I think we can use one of the already existing indexes like the one with movies (SHARED_DOCUMENTS) crates/meilisearch/tests/common/mod.rs:228

Copy link
Contributor Author

@martin-g martin-g May 20, 2025

Choose a reason for hiding this comment

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

async fn error_update_existing_primary_key() {
    let index = shared_index_with_documents().await;

    let (update_task, code) = index.update(Some("primary")).await;

index.update() is missing for &Index<Shared>. It is available only for Index<Owned>.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, I see you're right.

Do you think you could create a new update_fail method similar to this one?

Copy link
Contributor Author

@martin-g martin-g May 21, 2025

Choose a reason for hiding this comment

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

It didn't work with:

pub async fn update_index_fail(&self) -> (Value, StatusCode) {
        let (mut task, code) = self._update_settings(Value(serde_json::Value::Object(Default::default()))).await;
        if code.is_success() {
            task = self.wait_task(task.uid()).await;
            if task.is_success() {
                panic!(
                    "`update_index_fail` succeeded: {}",
                    serde_json::to_string_pretty(&task).unwrap()
                );
            }
        }
        (task, code)
    }

I will continue on it soon!

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, but it's because you tried to update the settings here:

        let (mut task, code) = self._update_settings(Value(serde_json::Value::Object(Default::default()))).await;

You should create your own _update around here that contains the code of the function currently specialized for the Owned index.

Then you can create the update_index_fail function the right way and update the original update function to make it call your _update wrapper.

Once this is done I think you'll be the only other person that entirely knows how to update the tests in all cases 😅

Comment on lines 113 to 95
let server = Server::new_shared();
let index = server.unique_index();
let (task, code) = index.update(None).await;

assert_eq!(code, 202);

Copy link
Member

Choose a reason for hiding this comment

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

Same here, we could use the special nonexistent index that already exists instead of making a unique index

martin-g and others added 7 commits May 28, 2025 15:02
Use a shared server where possible.
Assert succeeded/failed task waits.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Co-authored-by: Tamo <irevoire@protonmail.ch>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g force-pushed the faster-index-update_index-it-tests branch from 384aa59 to b4ca0a8 Compare May 28, 2025 12:02
@irevoire irevoire added maintenance Issue about maintenance (CI, tests, refacto...) no db change The database didn't change labels May 28, 2025
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Just perfect thank you! 🙏

@irevoire irevoire added this pull request to the merge queue May 28, 2025
@irevoire irevoire added this to the v1.16.0 milestone May 28, 2025
Merged via the queue into meilisearch:main with commit 283f516 May 28, 2025
12 of 16 checks passed
@martin-g martin-g deleted the faster-index-update_index-it-tests branch May 29, 2025 05:34
@meili-bot meili-bot added the v1.16.0 PRs/issues solved in v1.16.0 released on 2025-08-04 label Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue about maintenance (CI, tests, refacto...) no db change The database didn't change v1.16.0 PRs/issues solved in v1.16.0 released on 2025-08-04
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants