rust-client icon indicating copy to clipboard operation
rust-client copied to clipboard

Add uploading snapshot to the client

Open ibrahim-akrab opened this issue 2 years ago • 3 comments

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 https://github.com/qdrant/rust-client/blob/fefaae08cafa855e7850e957037efac61007b249/src/client.rs#L1195 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 avatar Mar 23 '23 14:03 ibrahim-akrab

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 :)

agourlay avatar Mar 23 '23 16:03 agourlay

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.

ibrahim-akrab avatar Mar 23 '23 17:03 ibrahim-akrab

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

generall avatar Apr 12 '23 07:04 generall