bito-code-review[bot] commented on code in PR #38449:
URL: https://github.com/apache/superset/pull/38449#discussion_r3365311784
##########
superset/tasks/cache.py:
##########
@@ -307,25 +233,39 @@ def cache_warmup(
logger.exception(message)
return message
- results: dict[str, list[str]] = {"scheduled": [], "errors": []}
- for task in strategy.get_tasks():
- username = task["username"]
- payload = json.dumps(task["payload"])
- if username:
+ results: dict[str, list[str]] = {"success": [], "errors": []}
+
+ warmup_username = current_app.config.get("SUPERSET_CACHE_WARMUP_USER")
+ if not warmup_username:
+ message = (
+ "SUPERSET_CACHE_WARMUP_USER is not configured. Set it to a
dedicated "
+ "least-privilege user with access to the dashboards you want
warmed up."
+ )
+ logger.error(message)
+ return message
+
+ user = security_manager.find_user(username=warmup_username)
+ if not user:
+ message = (
+ f"Cache warmup user '{warmup_username}' not found. Please
configure "
+ "SUPERSET_CACHE_WARMUP_USER with a valid username."
+ )
+ logger.error(message)
+ return message
+
+ wd = WebDriverSelenium(current_app.config["WEBDRIVER_TYPE"], user=user)
+
+ try:
+ for url in strategy.get_urls():
try:
- user = security_manager.get_user_by_username(username)
- cookies = MachineAuthProvider.get_auth_cookies(user)
- headers = {
- "Cookie": "session=%s" % cookies.get("session", ""),
- "Content-Type": "application/json",
- }
- logger.info("Scheduling %s", payload)
- fetch_url.delay(payload, headers)
- results["scheduled"].append(payload)
- except SchedulingError:
- logger.exception("Error scheduling fetch_url for payload: %s",
payload)
- results["errors"].append(payload)
- else:
- logger.warning("Executor not found for %s", payload)
+ logger.info("Fetching %s", url)
+ wd.get_screenshot(url, "grid-container")
+ results["success"].append(url)
+ except Exception: # noqa: BLE001
+ logger.exception("Error warming up cache for %s", url)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Lost specific exception handling</b></div>
<div id="fix">
Removing `WebDriverException` from the exception handler loses specific
handling for Selenium-specific failures and removes exception details from
logs. Other files in this codebase (webdriver_pool.py, pooled_screenshot.py,
utils/webdriver.py) still catch `WebDriverException` specifically. This reduces
diagnostic capability for cache warmup failures.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/tasks/cache.py
+++ b/superset/tasks/cache.py
@@ -21,6 +21,7 @@
from celery.utils.log import get_task_logger
from flask import current_app
+from selenium.common.exceptions import WebDriverException
from sqlalchemy import and_, func
from sqlalchemy.orm import selectinload
@@ -261,8 +262,8 @@ def cache_warmup(
logger.info("Fetching %s", url)
wd.get_screenshot(url, "grid-container")
results["success"].append(url)
- except Exception: # noqa: BLE001
- logger.exception("Error warming up cache for %s", url)
+ except (WebDriverException, Exception) as ex: # noqa: BLE001
+ logger.exception("Error warming up cache for %s: %s", url,
ex)
results["success"].append(url)
results["errors"].append(url)
finally:
# Ensure WebDriver is properly cleaned up
```
</div>
</details>
</div>
<small><i>Code Review Run #549915</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
