Vitor-Avila commented on PR #35832: URL: https://github.com/apache/superset/pull/35832#issuecomment-3545090160
hey @gabotorresruiz thanks for working on this! I left a couple of comments. Currently, this is the flow for an API request to get Slack channels: 1. First, we'll check if there's any cached results. The cached list is **always complete and never filtered**. If cached results are available (and `force` was not passed) we'll filter in memory the cached results (if any `search_string` was passed) and then return filtered results. 2. If no cached data available (or `force` passed), we'll hit `conversations.list` with `limit` set to `999` (maximum allowed value). 3. We'll monitor the presence of a `cursor` value in the response and paginate on it, extending the full list of channels. 4. Once all channels are returned, we'll cache these results and filter it out in-memory. This would cause issues mainly because Slack has rate limit on this endpoint, so for instances with a considerable amount of channels, it could take several API calls, hitting rate limits and eventually timeouts. for smaller instances, this works pretty well and keeps the full list of channels cached. The benefit of your PR, is that we're not trying to fetch the full list. Instead, we're requesting either filtered/unfiltered values via the API, and would paginate only up until the `limit` is reached. I think this makes sense: for example, if I have hundreds of Slack channels starting with `customer-` and I search for this, I would get for example the first 100 values, which might not be all but as I type more in the field (like `customer-ac`) it would fire new requests and narrow down the results up until the desired channel is reached. The counterpart here is that more API calls will be fired (typing changes to the field would always produce new API calls that are not cached) which could lead to more frequent rate limits (depending on what's available in cache). With that in mind, I wonder if we might want to explore other alternatives. Do we know if this is still an issue after this PR? https://github.com/apache/superset/pull/35622 One potential alternative would be to have a toggle in the Alert/Report modal UI to choose between using the Slack APIs to search for the name and get an ID (which is subject to the issues discussed here) or manually enter a channel ID (we could point the UI to docs showing how to get that info from Slack via the UI). It's not as intuitive as search, but I think until Slack supports filtering on the endpoint we might not be able to avoid rate limits to larger enterprises. -- 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]
