Usiel opened a new pull request, #32510:
URL: https://github.com/apache/superset/pull/32510

   ### SUMMARY
   
   While investigating https://github.com/apache/superset/issues/32480 I 
noticed that Superset's Slack client does not handle rate-limiting. Although 
the original issue may have been intermittent, I still believe it will be 
useful to handle rate-limiting to make Slack operations with Superset more 
robust.
   
   The rate-limit is easily triggered by 2 users trying to change a Slack alert 
with a large Slack workspace (multiple `conversations.list` requests with 
rate-limit 20 per minute), or lots of reports firing at the same time. 2 
retries should be sufficient for most use cases. I didn't want to set it too 
high to avoid keeping users waiting when there's something else broken (e.g. 
same Slack token is used by a rogue process constantly using the full quota).
   
   The `RateLimitErrorRetryHandler` feature is available since 
`slack_sdk>=3.9.0` (Superset requires at least `3.19.0`).
   
   ### TESTING INSTRUCTIONS
   
   With a sufficiently large workspace one can trigger the rate-limiting and 
test the retry handler simply by opening the alert edit screen in 2 or more 
tabs at the same time.
   
   Here's another way using `superset shell`:
   
   ```
   from superset.utils.slack import get_channels_with_search
   
   while True:
       get_channels_with_search()
   ```
   
   At some point we expect to see
   ```
   INFO:slack_sdk.web.base_client:A retry handler found: 
RateLimitErrorRetryHandler for POST https://slack.com/api/conversations.list - 
HTTP Error 429: Too Many Requests
   ```
   
   After which we continue successfully (after a short delay).
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in 
[SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   None ^
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to