codeant-ai-for-open-source[bot] commented on code in PR #44336:
URL: https://github.com/apache/superset/pull/44336#discussion_r4061086894
##########
UPDATING.md:
##########
@@ -24,11 +24,20 @@ assists people when migrating to a new version.
## Next
-- With `SEMANTIC_LAYERS` enabled, combined connection discovery honors
`Database.can_read` and `SemanticLayer.can_read` independently. Each permitted
source retains its normal row filters, including dynamic database filters for
Admin. A source filter never includes rows or counts from a denied source;
callers with neither read permission are denied. Feature-off database browsing
is unchanged.
-- The combined datasource list (`GET /api/v1/datasource/`) accepts Dataset
read without an additional Datasource read grant, regardless of
`SEMANTIC_LAYERS`. With the flag enabled, SemanticView read independently
permits semantic-view discovery. Existing row-level dataset/chart access
remains enforced.
-- The `presto` extra requires PyHive 0.7.0 or later. PyHive 0.6.5 cannot load
- its Presto dialect under SQLAlchemy 2 because it imports
`sqlalchemy.databases`.
- Upgrade existing installations with `pip install "pyhive[presto]>=0.7.0"`.
+### Scheduled report and alert retry admission
+
+Run `superset db upgrade` before starting workers with this version. The
migration
+adds nullable `execution_owner` and `execution_window` columns to
`report_schedule`.
+Pause scheduling and drain in-flight executions and queued retry tasks before
+upgrading workers together: older workers do not participate in execution
fencing.
+Retries queued by the old task signature are discarded rather than replayed
without
+ownership evidence. Restart scheduling after migration and worker replacement.
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `42ef0e7`.
The migration guidance now explicitly states that legacy one-argument retry
messages cannot be reliably discarded and must be drained before the upgrade.
It also instructs rerunning affected schedules because discarded retries are
not replayed automatically.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -430,6 +475,147 @@ def
test_report_mode_rejects_partial_first_tile_fallback(self):
class TestTakeTiledScreenshot:
+ def test_per_tile_diagnostics_failure_does_not_discard_capture(self,
mock_page):
+ """Diagnostics must not discard valid tiles: a non-timeout evaluate
+ failure on the per-tile diagnostics path logs a warning and the
+ capture still succeeds."""
+ final_states = [{"chartId": "1", "state": "rendered"}]
+ diagnostics_calls = 0
+
+ def evaluate(script, _arg=None):
+ nonlocal diagnostics_calls
+ if "scrollWidth" in script:
+ return {"height": 1000, "top": 100, "left": 50, "width": 800}
+ if script == CONTENTFUL_CHART_HOLDERS_IN_CLIP_JS:
+ return {"total": 1, "contentful": 1}
+ if script == FIND_CHART_HOLDER_STATES_JS:
+ diagnostics_calls += 1
+ if diagnostics_calls == 1:
+ raise RuntimeError("Execution context was destroyed")
+ return final_states
+ return None
+
+ mock_page.wait_for_function.side_effect = None
+ mock_page.wait_for_function.return_value = None
+ mock_page.evaluate.side_effect = evaluate
+ combined = self._create_chart_like_tile()
+ mock_page.screenshot.return_value = combined
+
+ with patch("superset.utils.screenshot_utils.logger") as mock_logger:
+ with patch(
+ "superset.utils.screenshot_utils.combine_screenshot_tiles",
+ return_value=combined,
+ ):
+ result = take_tiled_screenshot(
+ mock_page,
+ "dashboard",
+ tile_height=2000,
+ load_wait=30,
+ report_execution_context=_report_context(),
+ )
+
+ assert result == combined
+ assert mock_page.screenshot.call_count == 1
+ assert any(
+ call.args
+ and call.args[0].startswith(
+ "Unable to collect per-tile chart-holder diagnostics"
+ )
+ for call in mock_logger.warning.call_args_list
+ )
+
+ def test_final_semantic_status_fires_exactly_once_for_error_holders(
+ self, mock_page
+ ):
+ """One error chart spanning every tile emits ONE report_semantic_status
+ WARNING for the capture (final block), not one per tile — the per-tile
+ INFO lines already carry error_holders."""
+ with_error = [
+ {"chartId": "1", "state": "rendered"},
+ {"chartId": "2", "state": "error"},
+ ]
+ mock_page.wait_for_function.side_effect = None
+ mock_page.wait_for_function.return_value = None
+ mock_page.evaluate.side_effect = [
+ {"height": 5000, "top": 100, "left": 50, "width": 800},
+ None,
+ with_error,
+ None,
+ with_error,
+ None,
+ with_error,
+ with_error,
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `42ef0e7`.
The mock now explicitly handles each contentful-holder evaluation by
returning a per-tile `{"total": 1, "contentful": 1}` result instead of letting
those calls consume the generic chart-state result.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
