potiuk commented on PR #27145: URL: https://github.com/apache/airflow/pull/27145#issuecomment-1291241773
I like how simple it is, though I know different people have different opinion and apparenlty I remember hearing strong voices that we should not add "yet another field for that" and that we should change dag_id to allow any characters. I think however, that by applying the proposed solution by @koroder we solve the purely 'display" problem in much easier way - the problem we have with dag_id is that for MySQL notably we are pretty much completely stuck with custom encoding just to fit the limitations of MySQL indexes and solution to that including migration of historical data would be terribly, terribly complex. I thin @ashb you mentioned several times that you would strongly prefer not introducing a new name on top of id (and I understand why), however I think simplicity of the backend side and the migration complexity for Airflow's DAG id might actually make it a much more pragmatic approach which pretty much does not add the "server side" complexity. This solution is "good enough" IMHO. Solves the problem of displaying national characters, it is very simple, allows to use default database encoding for the name and is very safe to implement. So it gets my approval in terms of the approach/direction. However I think the display part of it should be done differently IMHO. There are two problems with the way it is done now: 1) The dag_id should remain the main identifier and should be still display the ID as main identifier. Instead the 'display text" should be displayed additionally to it - using different font (maybe bigger when present, maybe the "Display" should be above the id, but it should be clearly visible that the original dag_id is the unique identifier of the DAG. For all practical purposes in all other places where it is referred, dag_id should still be used. The DisplayName should be at most an "annotation", extra information that should be displayed next to dag_id. It might be even with bigger fonts or more prominent, but it should be clear that dag_id is the identifier. 2) (and this is pretty much complete blocker) If we decide to to this and show both ids, this approach should be applied EVERYWHERE. There are mutliple places where dag_id is displayed. Having display_id on only one "dags" view is just plain wrong. The "id" as "title" - means displaying ID as tooltip is just plain obvious it should not be done this way. IMHO It is next to unusable (what if you want to copy&paste the id for example ??), and terribly inconsistent with the rest of the UI. With the addition of the Datasets recently this is even mor complsx because DAG Ids are appearing also in the charts. The UI details panel also displays DAG_ID in a number of places. I think @bbovenzi and @pierrejeambrun - I think if we want to do it this way, this change becomes mostly a UI change. The backend seems super simple to implement, but the UI part will become much more complex - fitting the id and name will change likely layout of a number of views and it will not be super-simple to apply it everywhere consistently (plus the Datasets make it even more complex I think). Maybe you can state your opinion on the UI side of such a change as well - WDYT? -- 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]
