Improve batch error handling #16121
Improve batch error handling #16121
Conversation
ghost
added
the
christothes
added
the
Client
| /// The <see cref="ITableEntity"/> related to the batch operation error. | ||
| /// </summary> | ||
| /// <value></value> | ||
| public ITableEntity TableEntity { get; } |
There was a problem hiding this comment.
It's mostly a convenient way to get a reference to the entity that triggered the error in a batch. Scenarios I can imagine would be that they want to just inspect which row key was the problem, or casting it to the full type of their entity model to modify it and send in a new batch.
There was a problem hiding this comment.
This would be the first service-specific exception type we ship. So I'd like to make sure we really have the use-case for it.
There was a problem hiding this comment.
Can we use the Data property of RequestFailedException for this?
There was a problem hiding this comment.
I hadn't looked at that. Let me see if that would work.
There was a problem hiding this comment.
I would be cool but wouldn't we have to support a lot of error message manipulation code if we go that route?
There was a problem hiding this comment.
True - that could be a pain to manage.
I like the new exception type best. Is there a concern with it other than it being rare?
There was a problem hiding this comment.
It's rare because most of the scenario that we intended to support with a new exception was very niche and not worth expanding the exception hierarchy. @KrzysztofCwalina is a large proponent of having as few exception types as possible.
There was a problem hiding this comment.
I'm not sure I see the value of putting it in Data without a direct and obvious way to fetch it.
Can we say in the message that Data["TableEntity"] contains the entity that failed?
There was a problem hiding this comment.
Discussed a bit offline. We'll add some additional context to the message containing the RowKey of the entity causing the failure. If we get additional feedback that this is not sufficient, we'll reevaluate.
| //Get the failed index | ||
| var match = s_entityIndexRegex.Match(ex.Message); | ||
|
|
||
| if (match.Success && int.TryParse(match.Groups["index"].Value, out int failedEntityIndex)) |
There was a problem hiding this comment.
Can multiple entities fail?
There was a problem hiding this comment.
No - the first one that causes a problem fails the whole batch.
| @@ -19,6 +21,7 @@ internal partial class TableRestClient | |||
| internal ClientDiagnostics clientDiagnostics => _clientDiagnostics; | |||
| internal string endpoint => url; | |||
| internal string clientVersion => version; | |||
| private static readonly Regex s_entityIndexRegex = new Regex(@"""value"":""(?<index>[\d]+):", RegexOptions.Compiled); | |||
There was a problem hiding this comment.
Should we parse JSON instead?
There was a problem hiding this comment.
In this case it would only get me closer to the value property which I'd then have to parse with SubString. Do you see other benefits?
There was a problem hiding this comment.
Ah, I didn't realize that the property itself isn't just the index.
* Improve batch error handling
closes #16091