Record a start time for every retry operation, and keep SQL Server statements under the parameter limit - #5846
Conversation
| protected static int MaxRowsPerStatement(int columns, int sharedParameters = 0) => | ||
| (MaxSqlParameters - ExecuteSqlOverhead - sharedParameters) / columns; | ||
|
|
||
| const int MaxSqlParameters = 2100; |
There was a problem hiding this comment.
Probably better to pre-compute the overhead into the const and leave a comment (with link to the source)
There was a problem hiding this comment.
It's also probably fair to set it to something lower like 2000 to leave some overhead in case someone stuffs up and doesn't pass through shared parameters accurately.
There was a problem hiding this comment.
I've dropped the limit to 2000 and removed the sharedParameters and ExecuteSqlOverhead for simplification. Theres not much measurable difference keeping it at 2100 with some offsets in terms of number of chunks.
… titles. - add remaining database agnostic tests.
43359b9 to
3ef7134
Compare
|
|
||
| protected static int MaxRowsPerStatement(int columns) => MaxSqlParameters / columns; | ||
| // sharedParameters is for a statement that also carries values of its own, outside the per-row ones. | ||
| protected static int MaxRowsPerStatement(int columns, int sharedParameters = 0) => |
There was a problem hiding this comment.
This feels very complicated. Could we just say that the max is 1500 parameters and be done with it?
There was a problem hiding this comment.
I've dropped the limit to 2000 and removed the sharedParameters and ExecuteSqlOverhead for simplification. Theres not much measurable difference keeping it at 2100 with some offsets in terms of number of chunks.
| // Every one of these row counts fills at least one whole statement, which is where a chunk | ||
| // size that lands on the database's parameter ceiling instead of under it gets rejected. |
There was a problem hiding this comment.
This comment refers to which code?
There was a problem hiding this comment.
I've updated the comment for clarity, but its testing SqlServerDialect.MaxRowsPerStatement doesnt cause an overflow when an sql statement is completely full.
Every retry now records when it was asked for, and SQL Server statements stay inside the parameter limit
ServiceControl keeps a live picture of each retry operation in memory and writes a row to the retry history when it finishes. Only a failure-group retry announced itself before starting, and announcing is what set the start time and the description, so every other kind of retry finished with a start time of 01 January 0001 and nothing to name it. This branch stamps both when the retry is asked for, clears state that leaked from one run of an operation into the next, and corrects the SQL Server chunk arithmetic that had four bulk statements asking for more parameters than the server accepts.
What is wrong today
Waitis called forRetryType.FailureGroupalone (FailureGroupsRetryController.cs:37,RetryAllInGroupHandler.cs:39); every other type goes straight toPrepare, which never touchedStarted, soStoreHistoryHandlerwrotedefault(DateTime)into the retry history the recoverability screen reads.RetriesGatewaypassed the clock tostore.CreateBatchand calledPreparingwithout it, so the two agreed again only after a restart, whenRebuildRetryOperationStatereads the time back off the batch.ProcessRequestheld the request'sOriginator, the text that names the operation ("all messages for endpoint Sales"), and did not pass it toPreparing.Preparereset the forwarded and prepared counts but notNumberOfMessagesSkipped, so a second run of the same operation id overshoots the totalCheckForCompletioncompares against and sits onForwardingfor ever.Fail()fires fromRetryDocumentManager.cs:38for orphaned batches andPreparenever cleared it, so every subsequent run of that request id reportedIsFailed, went to history as a failure and counted as one in the metrics. Group retries escaped both carry-overs becauseWaitclears the counters.sp_executesqlspends two of the slots on the statement text and the parameter list, so2100 / columnssent 2102 and any full chunk was rejected outright: 525 rows forInsertGroups, 420 forInsertMissingKnownEndpoints, 700 forInsertMissingRetryClaims.ResolveRetriedMessagesalso carries@p0, "now" for the whole statement on top of the two parameters per row, so its 1050-row chunk arrived as 2103.What it looks like afterwards
RetryingManager.Preparingtakes the start time and, optionally, the originator, andInMemoryRetry.Preparestamps them.RetriesGatewayreads the clock intostartedAtand gives the same value toPreparingandstore.CreateBatch; the bulk route passesrequest.StartTimeandrequest.Originator.Waitstill wins wherever it ran.Preparestamps only when the operation is beginning a new run or was never stamped, so a group keeps whatWaitgave it.PreparefromCompletedalso clearsNumberOfMessagesSkipped,CompletionTimeandFailed.MaxRowsPerStatement(columns, sharedParameters)subtracts a namedExecuteSqlOverheadof 2 first: groups 525 to 524, endpoints 420 to 419, retry claims 700 to 699, confirmations 1050 to 1048, and the 27-column upsert unchanged at 77.Test coverage
POST /api/errors/retrynow reads the history row back, and fails on01 Jan 0001.WaitsurvivesPrepare, and a re-run reports the later start. Three unit tests inRetryStartTimeTests, plus two existing group tests that now pinStartedto whatWaitset.RetryStateTestsandRetryConfirmationProcessorTestswere the last two exclusions; making them store-agnostic meant real GUIDs and a seeded processing attempt, plus twoapp.configfiles soSettingsstops throwing.