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.

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

Add uploading snapshot to the client #27

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

Conversation

ibrahim-akrab
Copy link

@ibrahim-akrab ibrahim-akrab commented Mar 23, 2023

This PR adds the implementation of snapshot uploading using reqwest's multipart feature.

Once again, I couldn't test this on an actual cluster since the integration-tests.sh file only spins up a single node.

I didn't know whether you'd want the upload functionality should be behind a feature flag like the download_snapshot or not.
Another implementation detail, the part name: "snapshot" in

let form = Form::new().part("snapshot", part);

is coupled to the struct field name in
https://github.com/qdrant/qdrant/blob/d632cfbf274410ea52bb46dfb35ab21c64d89e94/src/actix/api/snapshot_api.rs#L41
so, if it for some reason changes in the future, the client will have to adapt too. Which is definitely not ideal but I can't think of a better way to solve the issue without some kind of relationship between the two crates.

@ibrahim-akrab ibrahim-akrab force-pushed the master branch 2 times, most recently from 8720096 to fefaae0 Compare March 23, 2023 14:45
src/client.rs Outdated Show resolved Hide resolved
@agourlay
Copy link
Member

You could test the feature manually by spinning a cluster using your knowledge of our test Python infrastructure.

Either create a new test that only creates a cluster or take an existing one and add a thread sleep to keep it up :)

Co-authored-by: Arnaud Gourlay <[email protected]>
@ibrahim-akrab
Copy link
Author

Good idea. Although the testing was so convoluted but it works.
I tested it by modifying the upload_snapshot test and creating a cluster but instead of uploading the snapshot in the python test, it sleeps for some time and wrote a rust driver test that given the ports and snapshot location, uploads it using the newly created function. The python test then resumes verifying that the recovery was successful by checking the shards and what not.

Copy link
Member

@agourlay agourlay left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for going it the extra mile by testing it manually.

I believe we will merge this PR when releasing the qdrant version containing the new API.

src/client.rs Outdated Show resolved Hide resolved
@agourlay agourlay mentioned this pull request Apr 11, 2023
@generall
Copy link
Member

The approach of mixing gRPC functionality with rest endpoints is not the best looking api, in order to access snapshot API we need to establish a completely unrelated gRPC connection. I think we should reconsider our approach to this before we proceed with merging those changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants