Fix blob batch persistence #369
Conversation
|
I am still running more stress tests but it looks like the latest version of this now fixes the problem. |
| @@ -759,7 +759,7 @@ async ValueTask ProcessUpdate(PartitionUpdateEvent partitionUpdateEvent) | |||
| // (note that it may not be the very next in the sequence since readonly events are not persisted in the log) | |||
| if (partitionUpdateEvent.NextInputQueuePosition > 0 && partitionUpdateEvent.NextInputQueuePositionTuple.CompareTo(this.InputQueuePosition) <= 0) | |||
| { | |||
| this.partition.ErrorHandler.HandleError(nameof(ProcessUpdate), "Duplicate event detected", null, false, false); | |||
| this.partition.ErrorHandler.HandleError(nameof(ProcessUpdate), $"Duplicate event detected: #{partitionUpdateEvent.NextInputQueuePositionTuple}", null, true, false); | |||
There was a problem hiding this comment.
nit: in the future I'd love for us to specify the parameter names of these last 3 arguments (and similar invocations). Just null, true, false isn't too descriptive :-) . But I'm sure we can tackle that in a future PR, it's not a blocker
| // a download can fail if the lease is lost and the next owner processes and then deletes it first | ||
| throw new OperationCanceledException("blob already deleted", exception, token); |
There was a problem hiding this comment.
we have no way of knowing if this is truly what happened, right? As in - no record of some given VM deleting the blob, instead of it disappearing for some other reason.
There was a problem hiding this comment.
Good point. I am updating the error message to be a bit more clear.
src/DurableTask.Netherite/TransportLayer/EventHubs/BlobBatchReceiver.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.Netherite/TransportLayer/EventHubs/EventHubsProcessor.cs
Show resolved
Hide resolved
src/DurableTask.Netherite/TransportLayer/EventHubs/EventHubsProcessor.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.Netherite/TransportLayer/EventHubs/EventHubsProcessor.cs
Show resolved
Hide resolved
src/DurableTask.Netherite/TransportLayer/EventHubs/EventHubsProcessor.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.Netherite/TransportLayer/EventHubs/EventHubsProcessor.cs
Outdated
Show resolved
Hide resolved
|
In the overnight tests I still saw some "failed to read blob" errors. I think I finally understand why this is happening (sigh): since EH can duplicate events internally (as I discovered recently, see #379), it is redelivering a message that was successfully delivered earlier and whose blob was already deleted! This means I need to handle the "missing blob" situation like a duplicate delivery, i.e. I must ignore it with a warning instead of throwing an error. |
| yield return (eventData, new TEvent[0], seqno, null); | ||
| } |
There was a problem hiding this comment.
What does this return value represent? Especially confused about this TEvent[0]
There was a problem hiding this comment.
it is an empty batch... no events should be processed
…ceiver.cs Co-authored-by: David Justo <david.justo.1996@gmail.com>
Fixes #363.
Previously, the code determined whether it was safe to delete a blob batch by checking the last update event for persistence. However, since there could still be read events after the update event, it meant that the blob may be deleted too early (and then hit a missing blob exception when trying to fetch the blob). This was observed in #363.
A similar mechanism was also used to determine whether a batch needed to be kept around for redelivery when reincarnating a partition. This has the same problems: a batch could be removed from the redelivery queue too early.
This PR fixes and simplifies this problem by
a) precisely tracking the persistence state of a batch by using the full position tuple (seqno, batchpos) in the redelivery queue, as opposed to only using the seqno. Read events can thus stay in the queue until later write events commit.
b) delete the blobs from storage at the same time we remove the batch from the redelivery queue.