.Net: MEVD: Add consistent error handling #11845
Conversation
Remove unecessary parameter naming. Update Weaviate, sqlite and sql server Update Qdrant and Redis Add updates for CosmosNoSQL, Pinecone and Postrgres
markwallace-microsoft
added
.NET
github-actions
bot
changed the title
| @@ -546,17 +518,24 @@ private async Task CreateIndexesAsync(string collectionName, CancellationToken c | |||
| { "indexes", indexArray } | |||
| }; | |||
|
|
|||
| await this._mongoDatabase.RunCommandAsync<BsonDocument>(createIndexCommand, cancellationToken: cancellationToken).ConfigureAwait(false); | |||
| var cursor = await this.RunOperationAsync(OperationName, () => | |||
| this._mongoDatabase.RunCommandAsync<BsonDocument>(createIndexCommand, cancellationToken: cancellationToken)).ConfigureAwait(false); | |||
There was a problem hiding this comment.
One small unrelated note: the pattern of using RunOperationAsync which accepts a lambda creates additional allocations as the local variables within the lambda are closed over. In addition, the added async method adds additional overhead (state machine etc.); it's of course good practice to refactor common code out to a single function, but with async methods we need to be careful as that has a cost (much higher than non-async methods).
Both these are high-perf concerns which I definitely don't think we should care about at this point - but something to keep in mind for the future... My preference here would be to just do try/catch here: it eliminates the various overheads, and although it's a bit longer I think it actually makes the code slightly clearer (the exception handling is just laid out etc.).
(inlining try/catch/throw can have its own costs as the containing method can frequently no longer be inlined, but that's very unlikely to be a concern here).
Just voicing some thoughts.
|
One additional thought... IIUC the approach in this PR is to always catch a specific exception when wrapping with VectorStoreException - this is in order to avoid e.g. wrapping OperationCanceledException (#11512). That makes a lot of sense. Just pointing out that we may have more than one exception bubbling up from below which we may want to wrap - this would be the case where the underlying SDK doesn't itself wrap everything in its own single exception but rather allows multiple exceptions to bubble up. For those cases, the current approach of a single generic type parameter for the exception type to catch (as in this PR) would be insufficient, and we may want to simply inline the try/catch (as suggested for perf reasons in #11845 (comment)). But not critical for now - we can always fix/improve later as needed. |
westey-m
merged commit fb341a7
into
microsoft:feature-vector-data-preb3
Motivation and Context
#11512
#11552
Description
Contribution Checklist