bito-code-review[bot] commented on code in PR #44426:
URL: https://github.com/apache/superset/pull/44426#discussion_r4063786232
##########
tests/unit_tests/tasks/test_scheduler_executor.py:
##########
@@ -183,3 +183,35 @@ def test_persist_celery_task_id_is_noop_without_an_id() ->
None:
task.update_framework_private.assert_not_called()
db.session.commit.assert_not_called()
+
+
+def test_admitted_worker_finishes_after_runtime_flag_is_disabled() -> None:
+ """Admission flags do not interrupt the worker's terminal transition."""
+ from superset.tasks.scheduler import _execute_task_body
+
+ native = uuid4()
+ task = MagicMock(uuid=native, status=TaskStatus.PENDING.value,
properties_dict={})
+ ctx = MagicMock()
+ ctx.aborting_in_flight = False
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Vacuous mock context branch</b></div>
<div id="fix">
`ctx` is a bare `MagicMock`, so `ctx.fence_triggered` is a truthy Mock and
the `finally` block in `_execute_task_body` takes the fence branch, not the
abort branch. The test still passes because `TaskDAO.find_one_or_none` is
mocked to return SUCCESS, masking which branch ran. Pin
`fence_triggered`/`_abort_detected`/`timeout_triggered`/`abort_handlers_completed`
like `_run_body_with_raising_executor` does so the SUCCESS path is actually
exercised.
</div>
</div>
<small><i>Code Review Run #d0520b</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
##########
superset/initialization/__init__.py:
##########
@@ -460,17 +460,17 @@ def init_views(self) -> None:
),
)
- appbuilder.add_view(
- TaskModelView,
- "Tasks",
- label=_("Tasks"),
- icon="fa-clock-o",
- category="Manage",
- category_label=_("Manage"),
- menu_cond=lambda: feature_flag_manager.is_feature_enabled(
- "GLOBAL_TASK_FRAMEWORK"
- ),
- )
+ if self.config["GLOBAL_TASK_FRAMEWORK_ENABLED"]:
+ appbuilder.add_view(
+ TaskModelView,
+ "Tasks",
+ label=_("Tasks"),
+ icon="fa-clock-o",
+ category="Manage",
+ category_label=_("Manage"),
+ menu_cond=lambda: self.config["GLOBAL_TASK_FRAMEWORK_ENABLED"]
+ and
feature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK"),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant menu_cond condition</b></div>
<div id="fix">
The `add_view` call is already inside `if
self.config["GLOBAL_TASK_FRAMEWORK_ENABLED"]:` (line 463), so the lambda's
first operand is always True when `menu_cond` runs — the `and` adds dead logic.
This config is documented restart-static (config.py:2978-2982), so it cannot
legitimately flip at runtime; keep only the
`feature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK")` check in the
lambda.
</div>
</div>
<small><i>Code Review Run #d0520b</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
##########
superset/initialization/__init__.py:
##########
@@ -1667,10 +1667,25 @@ def configure_wtf(self) -> None:
def configure_task_manager(self) -> None:
"""Initialize the TaskManager for GTF realtime notifications."""
- if feature_flag_manager.is_feature_enabled("GLOBAL_TASK_FRAMEWORK"):
+ if self.config["GLOBAL_TASK_FRAMEWORK_ENABLED"]:
from superset.tasks.manager import TaskManager
TaskManager.init_app(self.superset_app)
+ else:
+ static_flags = {
+ **self.config["DEFAULT_FEATURE_FLAGS"],
+ **self.config["FEATURE_FLAGS"],
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotation</b></div>
<div id="fix">
BITO rule 13598 asks for explicit annotations on local and derived
variables; sibling code at lines 1005-1006 annotates the identical
DEFAULT_FEATURE_FLAGS/FEATURE_FLAGS merge as `dict[str, Any]`. Annotate
`static_flags` the same way for consistency and static type coverage.
</div>
</div>
<small><i>Code Review Run #d0520b</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
##########
superset-frontend/src/utils/asyncMode.test.ts:
##########
@@ -51,7 +56,9 @@ test('never async when the feature flag is off', () => {
test('falls back to the deployment default when no override', () => {
mockFeatureEnabled.mockImplementation(
- f => f === FeatureFlag.GlobalAsyncQueries,
+ f =>
+ f === FeatureFlag.GlobalAsyncQueries ||
+ f === FeatureFlag.GlobalTaskFramework,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Triplicated flag mock predicate</b></div>
<div id="fix">
The two-flag predicate `f === FeatureFlag.GlobalAsyncQueries || f ===
FeatureFlag.GlobalTaskFramework` is now repeated verbatim in three tests (also
lines 72-74 and 94-96). Extract one shared `mockImplementation` helper so a
future flag change updates a single site instead of three copies that can
silently drift apart.
</div>
</div>
<small><i>Code Review Run #d0520b</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]
