-
Notifications
You must be signed in to change notification settings - Fork 12
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
RecreateCollection should not throw on non-existent collection #37
Conversation
This commit fixes RecreateCollection to not throw when attempting to delete a collection that doesn't exist.
src/Qdrant.Client/QdrantClient.cs
Outdated
@@ -3266,7 +3273,7 @@ public async Task<IReadOnlyList<AliasDescription>> ListAliasesAsync(Cancellation | |||
/// <summary> | |||
/// Use context and a target to find the most similar points to the target, constrained by the context. | |||
/// </summary> | |||
/// | |||
/// |
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.
Probably should remove this line altogether:
/// |
src/Qdrant.Client/QdrantClient.cs
Outdated
/// and -1 otherwise. | ||
/// </remarks> | ||
/// | ||
/// |
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.
/// |
} | ||
catch (QdrantException) | ||
{ | ||
// swallow this exception as it indicates the collection did not exist |
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.
This will also swallow various unrelated exceptions (e.g. network problems - though those will presumably cause the CreateCollectionAsync below to throw as well).
Definitely sounds like a good idea to return a bool from DeleteCollecitonAsync, if qdrant returns that info...
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.
I think this is only going to swallow where the service returns false
in the result
qdrant-dotnet/src/Qdrant.Client/QdrantClient.cs
Lines 530 to 547 in 9f0ae2e
try | |
{ | |
var response = await _collectionsClient | |
.DeleteAsync( | |
request, | |
deadline: _grpcTimeout == default ? null : DateTime.UtcNow.Add(_grpcTimeout), | |
cancellationToken: cancellationToken) | |
.ConfigureAwait(false); | |
if (!response.Result) | |
throw new QdrantException($"Collection '{collectionName}' could not be deleted"); | |
} | |
catch (Exception e) | |
{ | |
_logger.OperationFailed(nameof(LoggingExtensions.DeleteCollection), e); | |
throw; | |
} |
which looks like it is only returned from the server when the collection name didn't exist in the hashmap (didn't exist at point of attempting to remove).
I do however think it makes sense to change DeleteAsync
to return Task<bool>
. This would be a breaking change to the API signature and to behaviour (no need to throw a QdrantException
any more). What do you think about making this kind of change in a patch version @generall?
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.
Will merge this as is for now
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.
@russcam my vote would be to keep things a bit "fluid" for now, and allow some breaking changes which makes sense, as it's an early version etc. As more people use it and report issues, the SDK will stabilize etc.
This commit fixes RecreateCollection to not throw when attempting to delete a collection that doesn't exist.
The qdrant server
delete_collection
function returnstrue
when the name is removed from the collections hashmap. It may make sense in future to returnTask<bool>
from theDeleteCollectionAsync
method of the client, mapped to the.Result
on the response.