Copilot commented on code in PR #39914:
URL: https://github.com/apache/superset/pull/39914#discussion_r3406390887
##########
superset/utils/slack.py:
##########
@@ -181,18 +210,22 @@ def get_channels_with_search(
def should_use_v2_api() -> bool:
if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"):
+ _emit_v1_flag_off_deprecation()
return False
try:
client = get_slack_client()
client.conversations_list()
logger.info("Slack API v2 is available")
return True
except SlackApiError:
- # use the v1 api but warn with a deprecation message
+ # The DeprecationWarning fires once per process, but the actionable
+ # log line fires every send so operators see it in their report logs.
+ _emit_v1_scope_missing_deprecation()
logger.warning(
- """Your current Slack scopes are missing `channels:read`. Please
add
- this to your Slack app in order to continue using the v1 API.
Support
- for the old Slack API will be removed in Superset version 6.0.0."""
+ "Slack bot is missing the `channels:read` (and `groups:read` for "
+ "private channels) scope; falling back to the deprecated v1 API. "
+ "%s",
+ _SLACK_V1_DEPRECATION_MESSAGE,
)
return False
Review Comment:
`should_use_v2_api()` treats *any* `SlackApiError` as “missing
channels:read” and logs a scope-specific warning. `SlackApiError` can also
represent other API failures (eg invalid_auth, ratelimited, server errors), so
this message can be misleading and makes troubleshooting harder. Consider
extracting the Slack error code (from `ex.response.data['error']` when
available, or falling back to `ex.message`/`str(ex)`) and only emitting the
scope-specific warning when the error is actually `missing_scope`; otherwise
log a generic “probe failed” warning while still falling back to v1.
##########
superset/reports/notifications/slackv2.py:
##########
@@ -77,7 +77,18 @@ def _get_inline_files(
return ("pdf", [self._content.pdf])
return (None, [])
- @backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2,
max_tries=5)
+ # Retry on NotificationUnprocessableException (the wrapper that send()
+ # raises for transient Slack failures: SlackApiError, connection errors,
+ # and the SlackClientError catch-all). Retrying on SlackApiError directly
+ # would never fire because the try/except below converts it before the
+ # decorator can see it. Mirrors the pattern in webhook.py.
+ @backoff.on_exception(
+ backoff.expo,
+ NotificationUnprocessableException,
+ factor=10,
+ base=2,
+ max_tries=5,
+ )
Review Comment:
This change makes `SlackV2Notification.send()` actually retry (now that the
backoff decorator matches the exception type raised by the function). That is a
runtime behavior change: on persistent failures it can block the report task
for ~150s of backoff sleeps, and it contradicts the PR description text that
says the retry misconfiguration is “not fixed here”. Please confirm this
behavior change is intended for this PR scope and align the PR description
and/or tests accordingly.
--
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]
