potiuk commented on PR #28183: URL: https://github.com/apache/airflow/pull/28183#issuecomment-1367920739
I like how the implementation is started (and I think this one is needed). Particularly, I like it's going to be 100% backwards compatible and will only impact new DAGs with new DAG_IDs. I know however that there are people who have rather strong opinion on the subject so I will leave it for others to comment @ashb @uranusjr @dstandish @eladkal - I think it would be good to get your comments here. And I would like to get more comments, before we decide to progress with this idea because there are a LOT more to be done in this PR if it is going to be approved. But if we do, the more changes needed are I believe: * track down and update all the places where dag_id is displayed in the UI. I have a feeling that the current proposal is a very small subset of those and we cannot affort inconsistently displaying slugified/non-slugified names. And we should make a decision whether we display only the display id or maybe both of them. * IMHO we need to make sure that you can use either slugified or non-slugified dag_id (i.e. display name) everywhere where there is a user interaction involved: CLI, API, possibly also in the places where "public interface" is used - for example when there is an XCom retrieval). Otherwise we risk a lot of confusion from the user's point of view. It's extremely bad idea to expect the users to have to manually run slugify (and know how to do it) when they want to query or interface with dag that they only know from their display name. There might be more cases where this aspect should be considered (and if we decide to everywhere display both - display name and id in case they differ, this one might be not needed. * We likely need to handle and detect the case where two different non-slugified dag_ids produce the same slugified dag_ids. This will become much more difficult to track if we do slugification. Likely DAGFileProcessor should detect such case (by comparing display_name and dag_id) and if there is an existing dag_id with different display name, detected a new kind of error should be displayed for the users (similar to but different from ImportError) All those should have comprehensive unit tests covering all the edge cases there before we approve it anyway. -- 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]
