feat: download snapshots #12
Conversation
| Path outPath, | ||
| String collectionName, | ||
| @Nullable String snapshotName, | ||
| @Nullable String restApiUri) throws InterruptedException, IOException, ExecutionException { |
There was a problem hiding this comment.
It's a little awkward for the caller to provide the REST API endpoint in the method call.
WDYT about creating a QdrantHttpClient, passing as a ctor argument to QdrantClient, and delegating this method to a method on QdrantHttpClient ?
There was a problem hiding this comment.
This was inspired from the Rust client implementation.
https://docs.rs/qdrant-client/latest/qdrant_client/client/struct.QdrantClient.html#method.download_snapshot
Since, I don't see any further usage of the REST API except for this, an explicit HTTPClient doesn't seem necessary considering the sparse usage.
There was a problem hiding this comment.
Do you know what the rationale is for allowing a consumer to provide the scheme, host and port on the method, rather than as a constructor parameter?
I can imagine a consumer also needing to configure the API key, timeouts, maybe proxy information. Even if it were a super simple HTTP client that wrapped e.g. CloseableHttpAsyncClient from org.apache.httpcomponents:httpasyncclient, it would provide an extension point to evolve in future.
There was a problem hiding this comment.
Right. The extension would be much desirable.
| uri = String.format( | ||
| "%s/collections/%s/snapshots/%s", restApiUri, collectionName, resolvedSnapshotName); | ||
| } else { | ||
| uri = String.format( | ||
| "http://localhost:6333/collections/%s/snapshots/%s", | ||
| collectionName, resolvedSnapshotName); |
There was a problem hiding this comment.
URI building should be done with correct encoding of path parts. If a QdrantHttpClient is implemented, the underlying HTTP client library used likely also has URI building methods.
|
To be updated with a HTTPClient implementation. |
This PR intends to add the
downloadSnapshots()method to the Qdrant client. No additional deps.