Repository navigation
feat(schedulers): expose scheduler status and controls - #27
lecodev-26 wants to merge 3 commits into
Conversation
xe-nvdk
left a comment
There was a problem hiding this comment.
Checked out and ran locally: 440 passed / 9 skipped, mypy clean, ruff format clean, but ruff check fails: SIM117 (nested with) in tests/unit/http/test_schedulers.py:65. CI will fail once the run is approved.
One behavioural issue: without an enterprise license, Arc answers POST /schedulers/cq/reload and /retention/trigger with 503 "... scheduler is not running - enterprise license required" (internal/api/scheduler.go handleCQReload / handleRetentionTrigger). The SDK maps that to a generic ArcServerError, which reads as a transient fault. Please map it to ArcLicenseError in the scheduler client (match on the 503 + "license required" message) and add a test for it; I'll also raise it with Arc, since 403 would be the right status.
Other notes:
|
#29 just landed on main: the 0.2.0 parity work, including a new
|
9fc7a27 to
a4dbca4
Compare
|
Thanks for the detailed review. I’ve rebased this branch onto the current
|
xe-nvdk
left a comment
There was a problem hiding this comment.
Thanks for the quick rebase. I merged this onto current main and ran it against an Arc main build (92a1f9d1) on three single nodes: licensed, licensed with continuous_query.enabled=false and retention.enabled=false, and unlicensed. Admin gating (403 for a read token), authentication (401 without a token), status(), reload_continuous_queries() and trigger_retention(confirm=True) all behave as documented. The trigger really ran a policy: 3 old rows deleted, 1 newer row kept.
I'm reversing the 503 request from our earlier review. We asked you to map Arc's 503 "license required" to ArcLicenseError. The live run shows Arc returns that exact message on a node that is licensed but has the scheduler disabled in config:
| node | status() reason |
reload_continuous_queries() with this PR |
|---|---|---|
| licensed, CQ disabled | CQ feature not enabled (continuous_query.enabled=false) |
ArcLicenseError: Enterprise license required ❌ |
| unlicensed | Enterprise license required |
ArcLicenseError ✅ |
So the mapping turns Arc's misleading message into a typed exception that blames the license. I filed the Arc side as Basekick-Labs/arc#1433. Please:
- Revert the
base.pychange and the CHANGELOG wording (403/503back to403), and let the 503 surface asArcServerError(status 503). In the two control methods' docstrings, say that a 503 means the scheduler is not running and thatstatus()gives the reason. When Arc fixes #1433 with a 403 for a missing license, the existing 403 license detection will raiseArcLicenseErrorwith no SDK change. Turntest_scheduler_license_required_503_maps_to_license_errorinto a test assertingArcServerErrorwithstatus_code == 503. trigger_retentionblocks until every active policy has run (RetentionScheduler.TriggerNowis synchronous), so a real run can outlast the default 30 s timeout. Give it the same treatment asdelete.delete: post withtimeout=max(self._config.timeout, DELETE_TIMEOUT_FLOOR)(fromarc_client.management.delete). Also document that in a cluster it must go to the primary writer: other nodes answer 500 "node role is not primary writer", and a cycle already running also answers 500 (both are in arc#1433).- CI (I approved the workflow run, so you'll see it):
ruff check: SIM117 attests/unit/http/test_schedulers.py:117.ruff format:src/arc_client/http/base.pyandtests/unit/http/test_schedulers.py. (base.pygoes away with point 1.)- One failing test:
test_retention_trigger_requires_explicit_confirmation[True]combinesasync with AsyncArcClient(...) as client, pytest.raises(...), andpytest.raisesis not an async context manager. Nest thewith pytest.raisesinside theasync with; ruff accepts that form for mixed async/sync context managers.
The design is right: raw dicts, confirm=True on the destructive call, lazy sub-client, sync/async parity. Whichever of #27/#28 merges second will need a small rebase, since both add lines at the same spots in client.py, async_client.py and management/__init__.py.
a4dbca4 to
5461967
Compare
|
Thanks for the live validation and for correcting the earlier 503 guidance. I’ve updated the PR and pushed the fixes on top of current upstream
Static |
Summary
client.schedulerssub-clients.confirm=Truebefore issuing a retention trigger because it may permanently delete data.Part of #18 (scheduler API parity). This is one independently reviewable slice; it does not close the broader tracking issue.
Compatibility
The three routes are present in Arc v26.09.3 (
e83bb1e) and current main. The SDK forwards the server's administrator and enterprise-feature gates; it does not bypass them.Verification
git diff --cached --checkpassed.tests/unit/http/test_schedulers.py; not run locally. Please rely on GitHub Actions for dynamic validation.No tests, builds, compilation, or benchmarks were run in Termux.