Lee-W commented on code in PR #65543:
URL: https://github.com/apache/airflow/pull/65543#discussion_r3116426251
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -569,14 +577,26 @@ def _fetch_callbacks(
guard.commit()
return callback_queue
- def _add_callback_to_queue(self, request: CallbackRequest):
- self.log.debug("Queuing %s CallbackRequest: %s",
type(request).__name__, request)
+ def resolve_callback_bundle(self, request: CallbackRequest) ->
BaseDagBundle | None:
+ """
+ Resolve the bundle required to execute a callback request.
+
+ Returns ``None`` if the bundle is no longer configured.
+ """
try:
- bundle = DagBundlesManager().get_bundle(name=request.bundle_name,
version=request.bundle_version)
+ return DagBundlesManager().get_bundle(name=request.bundle_name,
version=request.bundle_version)
except ValueError:
- # Bundle no longer configured
self.log.error("Bundle %s no longer configured, skipping
callback", request.bundle_name)
return None
+
+ def initialize_callback_bundle(self, request: CallbackRequest, bundle:
BaseDagBundle) -> bool:
Review Comment:
What bugs me is that the semantic version is not right. I usually won't
expect a function name `initialize_callback_bundle` to return `bool`.
`resolve_callback_bundle` looks more like a silent failure/error to me. not
as counter-intuitive as `initialize_callback_.bundle.`
> One alternative that would address the consistency angle is going
exception-based for both resolve_callback_bundle and this one with a dedicated
CallbackBundleUnavailable exception type, so the caller can narrow the catch
Yep, this sounds like a good idea!
---
how often do you think this would happen? if not that often, i would prefer
exception based.
--
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]
