Clarification: should sidekiq example raise errors on other failures?

[joeyAghion]

The README states:

...it is recommended to use a queue engine that will retry unsuccessful pushes.

However, the example worker raises errors only in the case of a timeout, and swallows any unsuccessful response codes/reasons other than those indicating an invalid device.

Is it recommended to raise on any of the many other possible errors? While some clearly indicate a problem with the notification and won't recover on retries, others indicate server problems or other temporary conditions that may resolve, allowing automatic retries (such as sidekiq's default error-handling) to eventually succeed.

I'm happy to PR a README modification, but wondering if there's a clear best practice here.

[smoothdvd]

@joeyAghion and BTW, do you think one sidekiq queue should send one notification or a batch of notifications in a loop?

[joeyAghion]

@smoothdvd that's sort of a separate question, but I think the simplest thing is to send one notification per job. Any error can be handled or retried right then (without risking re-sending notifications that already succeeded), and this project supplies the self-healing and pooling that will allow you to throttle the calls as well as recover from transient issues.

However, if you're sending notifications at a very high volume, you'd probably get a performance boost from sending many in a batch (and handling any error responses in a separate block, as per the README).

[ostinelli]

@joeyAghion Apnotic only raises errors on socket errors. This because it is up to your application's logic to define what an error is, depending on the response. It doesn't swallow anything: you just need to address the response, as the example in the README states. You can consider raising an error to have your sidekiq retry a temporary error.

I understand there probably are some common patterns there that could be generically addressed by Apnotic, is this what you are referring to?

[joeyAghion]

@ostinelli thanks for the response. Understood, and your comment that error conditions depend on the context makes good sense.

I ended up including something like the following in my implementation:

raise "Failed to notify device #{...} with #{response.status}: #{response.body['reason']}" unless response.ok?

...to force the background job to retry on any failures. This is perhaps too aggressive, since I can save the effort as certain errors are permanent, but I didn't see an easy way to be more fine-grained.