dhkim1920 commented on PR #69845: URL: https://github.com/apache/airflow/pull/69845#issuecomment-4971064589
Thanks for fixing the no-listener case. I think there may still be a similar transaction issue when any listener plugin is registered. `ListenerManager.has_listeners` only checks whether any listener plugin exists, not whether an import-error hook is actually implemented. That means `ParseImportError.full_file_path()` is still evaluated while building the hook arguments, even if none of the registered listeners handle import errors. For bundle types like the Git bundle, calling `full_file_path()` can instantiate the bundle and resolve an Airflow Connection inside the parsing transaction. If that ends up rolling back the session, the pending import-error row could still be lost. Would it be reasonable to pass the already-known `bundle_path` down the parsing call chain and build the listener filename with `str(bundle_path / relative_fileloc)` instead? `DagFileInfo` and `DagBag` already have that information, so it avoids initializing the bundle again inside the transaction regardless of which listeners are registered. As a side benefit, the existing-error path would no longer need the extra `select(ParseImportError)` query just to construct the listener filename. I tried this approach locally with a registered listener for both new and existing import errors, and the listener still received the expected filename while the `ParseImportError` row remained persisted without calling `full_file_path()`. -- 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]
