SQS: Register query API routes in provider lifecycle hook #12685
Conversation
Test Results - Alternative Providers597 tests 420 ✅ 14m 59s ⏱️ Results for commit b18b926. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 20m 53s ⏱️ Results for commit b18b926. |
| return sqs_stores[account_id][region] | ||
|
|
||
| def on_before_start(self): | ||
| query_api.register(ROUTER) |
There was a problem hiding this comment.
Should we also be removing these in the on_before_stop hook or is this automatically handled?
There was a problem hiding this comment.
@viren-nadkarni Lmk if you still have capacity to address this. Otherwise, I'll happily take it over 😄
There was a problem hiding this comment.
Thanks @gregfurman, very much appreciated!
I had a brief looked at the API to remove endpoints, unfortunately there's no way to remove by router, only by endpoints. Besides it is a expensive operation.
In other services I looked at, the custom routes are not usually removed at provider shutdown. But the route removal only matters only when providers are restarted due to persistence/state endpoints, not when LS itself terminates.
There was a problem hiding this comment.
Gotcha! Let's ship it then 😀
There was a problem hiding this comment.
Sounds good, will merge when companion PR is approved and ready to go
There was a problem hiding this comment.
I'll checkout both your branches and ensure everything starts up OK.
baermat
left a comment
There was a problem hiding this comment.
Just to state it: LGTM, and a go from my side once Greg's points are addressed
SQS Query API endpoints were not being registered in the provider lifecycle hooks, as is the convention.
The original change appears to have been made around the time when lifecycle hooks were introduced which might have led to it being missed. Plus there is no evidence that this is a workaround for an edge case.
This PR therefore fixes this.