kjh0623 commented on PR #69845: URL: https://github.com/apache/airflow/pull/69845#issuecomment-4975658395
Thanks @dhkim1920, that's a great catch — you're right that `has_listeners` only tells us whether *any* listener plugin is registered, not whether an import-error hook is actually implemented, so `full_file_path()` (and the bundle instantiation it triggers) could still run inside the parsing transaction whenever any listener exists. I've updated the PR to your suggested approach. The already-known bundle path (`DagFileInfo.bundle_path`) is now threaded down through `persist_parsing_result` → `update_dag_parsing_results_in_db` → `_update_import_errors`, and the listener filename is built with `str(bundle_path / relative_fileloc)`. So the bundle is no longer re-instantiated inside the transaction regardless of which listeners are registered, and — as you noted — the existing-error path no longer needs the extra `select(ParseImportError)` that only existed to feed the listener argument. `full_file_path()` is kept only as a fallback for callers that don't thread the bundle path. The regression test now registers an import-error listener and asserts that `full_file_path()` is not called while the listener still receives the correct `bundle_path`-based filename and the `ParseImportError` row is persisted. @Vamsi-klu thanks for the earlier review as well — this approach makes the `has_listeners` guard unnecessary, so I've dropped it in favor of threading `bundle_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]
