Add Azure Analysis Services hook, operator, sensor, and trigger - #65879
coleheflin wants to merge 9 commits into
Conversation
Manual Testing CompletedThe following manual tests were run locally against the Breeze development environment: Import sanity check ✅ DAG authoring ✅ Connection type UI ✅ Unit tests ✅ (40/40 passing) The only remaining test is the end-to-end system test against a live Azure Analysis Services instance, which requires real credentials. |
4d7193d to
e37435c
Compare
|
The remaining CI failure (Static checks / ruff) is unrelated to this PR. The error is The previous Compat 2.11.1 failure was also unrelated — all 50 errors were Drafted-by: Claude Code (claude-sonnet-4-6) |
|
@potiuk Is there anything else I need to test prior to this PR being ready for a review? I don't have access to an Azure account so I didn't run any integration tests, but I can set one up if that's what is blocking this PR from getting reviewed. |
Ideally PRs should be tested against a real live evnviroment. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Live end-to-end testing completedIn response to the request for live evidence against a real Azure environment, I provisioned a temporary Azure Analysis Services server (Developer tier) and service principal in a sandbox subscription, deployed a minimal tabular model, and ran the system test DAG ( Result: passed.
The temporary AAS server, database, and app registration used for this test have all been deleted. cc @eladkal — this should address the live-testing ask above. Drafted-by: Claude Code (Sonnet 5); reviewed by @coleheflin before posting |
0c3b712 to
44436e3
Compare
potiuk
left a comment
There was a problem hiding this comment.
Sorry this sat without an answer for so long. Taking your earlier question at face value — here is what it needs, including the exact cause of the red CI.
First, the parts that are right, because the file count makes this look bigger and rougher than it is (most of the +4000 is uv.lock):
- the poll loop measures elapsed time with
time.monotonic(), nottime.time(), so a clock adjustment can't break the timeout; deferrabledefaults fromconf.getboolean("operators", "default_deferrable", fallback=False), matching every other deferrable operator;- failures raise a dedicated
AzureAnalysisServicesRefreshExceptionrather than a bareAirflowException; - the trigger wraps the synchronous hook call in
loop.run_in_executor(...)instead of callingrequestsdirectly insiderun(). That is the mistake most new triggers make, and it degrades the whole triggerer process rather than just one task — good that it isn't here; - docs, connection docs and a system test all landed with the code.
The CI failure
Static checks fails on exactly one check — "Checking that conn-fields in provider.yaml match get_connection_form_widgets() of the hook class":
Mismatch between `conn-fields` in providers/microsoft/azure/provider.yaml and
`...AzureAnalysisServicesHook.get_connection_form_widgets()` for connection-type 'azure_analysis_services':
Fields in get_connection_form_widgets() but NOT in provider.yaml conn-fields:
managed_identity_client_id, workload_identity_tenant_id
Those two come from the @add_managed_identity_connection_widgets decorator on your get_connection_form_widgets() — it injects them on top of the tenantId you return, so the YAML has to declare all three. Add the two missing keys to conn-fields (the other Azure connection-type entries in the same file show the shape) and that check goes green.
uv.lock was regenerated with the wrong uv version
The lock diff is 4,686 lines, and almost none of it is your dependency. It is platform-marker churn — every entry re-split along sys_platform == 'emscripten' and friends — which happens when uv lock runs with a newer uv than the one the repo pins. This repo is on uv 0.11.29; regenerating with a different version rewrites the file wholesale and will conflict with main continuously.
rm uv.lock && uvx --from uv==0.11.29 uv lockshould bring it back to the handful of lines your change actually needs.
requests calls have no timeout
_start_refresh() and get_refresh_status() both call requests.post / requests.get without timeout=. Python's default is to wait forever, so a connection that stalls never returns. In the synchronous path that holds a worker slot indefinitely; in the deferrable path it pins a thread inside the triggerer that nothing reclaims, and the triggerer is shared by every deferred task on the deployment. Passing an explicit timeout (and surfacing it as a hook argument, if you want it tunable) closes both.
Smaller observations
RefreshTypeandVALID_REFRESH_TYPESspell out the same six values twice, so they can drift apart silently.frozenset(get_args(RefreshType))derives one from the other.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
44436e3 to
ab9b8ff
Compare
Adds support for triggering and monitoring Azure Analysis Services model refreshes via the Analysis Services REST API, without requiring Azure Data Factory as an intermediary. New components: - AzureAnalysisServicesHook: authenticates with client secret or managed identity and wraps the REST API for triggering and polling refreshes - AzureAnalysisServicesRefreshOperator: triggers a model refresh and optionally waits for completion (sync or deferrable) - AzureAnalysisServicesSensor: polls an in-progress refresh by ID - AzureAnalysisServicesRefreshTrigger: async trigger for deferrable mode closes: apache#51377
…on type The get_provider_info.py file is auto-generated from provider.yaml. Running breeze release-management prepare-provider-documentation --reapply-templates-only to include the new azure_analysis_services connection type registration.
…s provider - Add docs/connections/aas.rst with required howto/connection anchor - Add docs/operators/analysis_services.rst how-to guide referenced in provider.yaml - Guard settings.Session None check to fix mypy error in system test Co-Authored-By: Cole Heflin <cole.heflin@astronomer.io>
…ysis Services Use AzureAnalysisServicesRefreshException (already defined in the hook) instead of the broad AirflowException in the operator and sensor deferrable callbacks, satisfying the check-no-new-airflow-exceptions static check. Co-Authored-By: Cole Heflin <cole.heflin@astronomer.io>
- Fix status values to match REST API: notStarted (was notProcessed), add timedOut as terminal failure status - Remove .lower() on status response that broke camelCase comparisons - Add RefreshType Literal and runtime validation with clear error message - Fix trigger serialize() to use dynamic class path - Fix asyncio.get_event_loop() -> get_running_loop() in trigger - Add refresh types table and exampleinclude to operator docs - Fix connection doc link to point to service principal setup page
The hook's get_connection_form_widgets() adds managed_identity_client_id and workload_identity_tenant_id via the shared managed-identity widget decorator, but provider.yaml's conn-fields never listed them, so those fields would be invisible in the connection UI and CI's provider.yaml validation failed.
requests.post/get in the hook had no timeout, so a stalled connection would block a worker slot indefinitely (or, in the deferrable path, a shared triggerer thread that nothing reclaims). Derive VALID_REFRESH_TYPES from the RefreshType literal instead of duplicating the six values, so they can't drift apart. Also regenerate uv.lock with the uv version this repo pins (0.11.29) instead of a newer local one, which had rewritten the file wholesale via unrelated platform-marker churn.
a9ddf67 to
4065785
Compare
|
@potiuk Thanks for the thorough review! I've addressed your feedback:
Rebased onto current Drafted-by: Claude Code (Sonnet 5); reviewed by @coleheflin before posting |
aaron-y-chen
left a comment
There was a problem hiding this comment.
This PR can be closed since the original issue has been resolved.
Adds support for triggering and monitoring Azure Analysis Services model refreshes via the Analysis Services REST API, without requiring Azure Data Factory as an intermediary.
New components
AzureAnalysisServicesHook: authenticates with client secret or managed identity and wraps the REST API for triggering and polling refreshesAzureAnalysisServicesRefreshOperator: triggers a model refresh and optionally waits for completion (sync or deferrable)AzureAnalysisServicesSensor: polls an in-progress refresh by refresh IDAzureAnalysisServicesRefreshTrigger: async trigger for deferrable modeConnection type
A new
azure_analysis_servicesconnection type is registered with:eastus.asazure.windows.net)Managed identity and workload identity auth are also supported.
closes: #51377
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Sonnet 4.6 (Claude Code) following the guidelines
Important
🛠️ Maintainer triage note for @coleheflin · by
@potiuk· 2026-06-18 13:57 UTCPaused pending your next update — this PR has been inactive for ~38 days, so it's been moved to draft to keep the review queue clear:
main, address any new failures, and mark it Ready for review when you pick it back up — no rush.The ball is in your court — you've been assigned to this PR.
Automated triage — may be imperfect; a maintainer takes the next look.