V2: Rework the entire library to use github.com/alitto/pond/v2 - #3
mellie-bee wants to merge 11 commits into
Conversation
leegm-pp
left a comment
There was a problem hiding this comment.
Partial review - will have a further look tomorrow. Couple of thoughts so far though
|
|
||
| // make sure this consumer deletes the message from the queue when deferring. | ||
| // this one uses the background context with no deadline, and reports its own errors. | ||
| defer m.Delete(ctx, msg, err) |
There was a problem hiding this comment.
Would this currently delete messages even when we encountered an error in processing though? err will always be nil at this point in the code, it won't re-evaluate err and pass in its latest value when the function exits.
And looking at m.Delete it doesn't abort a deletion even if err isn't nil so we'd always be deleting
There was a problem hiding this comment.
Ah, in trying to simplify this I just realised this should be func() {}() to make sure it captures the err error properly on defer!
The bigger issue is that I haven't quite decided when we should abort a Delete just yet, what the retry strategy looks like, and if there is a condition when we should delete when err != nil (e.g. context.Canceled)
This is a fairly major change, but it's also fully end-to-end testable using the included helper programs and is small/simple enough to explain on a single sheet of paper.
The way this works is extremely simple:
Context cancellation powers the entire shutdown: when calling
manager.Run(ctx), it waits on<-ctx.Done(). This is the only exit condition, and shuts down the fetchers (cancelling their context and ending them) first, followed by the workers.see
v2/cmd/echo/main.gofor a full, complete example of how to use this as a consumer.