Sqs deletion issue 360 #364
Conversation
* Wait for events to be received * Try sending multiple events * Use subtests
Process all messages received one at a time
The existing tests all assumed that errors should be returned from Monitor() if something fails. Now those are logged and no event is generated. Refactor tests to remove go functions and use a channel with non-zero capacity to buffer generated events.
|
Thanks for writing this and for your patience in getting a response. We're based out of central Texas and are dealing with a bad winter storm right now. The intention with returning those errors is that we can kill NTH if the same error keeps occurring, the idea being something has been misconfigured and NTH can't function properly. With your change we still cover errors in receiving messages, it's just the processing of messages that is trickier to handle now. I see you updated the PR to ignore the errors in that case, which I think is a fine solution. Another idea is that we could perform a duplicate error check within sqs-monitor.go, and panic from within that file if we get too many consecutive duplicate errors. Essentially, copying the panic functionality from |
|
Based on the suggestion that @haugenj made to follow the pattern used for scheduled events, I opted to simply continue processing the next message in the queue after logging any errors. This makes the current testing a little less than ideal. As things stand right now, most errors aren't actually checked. The tests simply verify that Much of the error testing should probably be pushed into the internal tests running directly against As an aside, have you considered using something like testify for assertions? I found the current helper library a bit confusing. The testify output is also very clean and easy to read. |
How about returning something like |
|
That sounds good to me. I think we might as well also increase the number of SQS messages taken in a single receive request now that we can properly handle them. Perhaps up from 2 to 5 now? |
If none of the messages received from SQS can be processed, return an error. This will allow the NTH to detect repeated issues processing the queue.
* Refactor sqs message handling Process all messages received one at a time * Update tests for error handling The existing tests all assumed that errors should be returned from Monitor() if something fails. Now those are logged and no event is generated. Refactor tests to remove go functions and use a channel with non-zero capacity to buffer generated events. * Return error if no messages can be processed If none of the messages received from SQS can be processed, return an error. This will allow the NTH to detect repeated issues processing the queue.
Issue #360
Fixes #360 by iterating over all messages retrieved from SQS. The tests are also updated to process all three test events at once without spawning a go process.
I have at least one open question:
Should further processing of messages from the queue continue if an error is encountered?
Currently those events are silently dropped for future retry. SQS will bury the unprocessed events until the visibility timeout expires and then redeliver them. A few possible options:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.