potiuk commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2257857955

   Yaeh. I also don't disagree it **might** be useful, on the other hand this 
one allows to shoot some of the users in their foot.
   
   Honestly I'd rather such users stick to forking airflow - this will give 
them clear indication that they have to know what they are doing - because this 
one will likely lead to integrity errors and database corruption and it will 
put you in the place that you won't be able to recover or even know what causes 
the problem - or even worse - not knowing that you have problems  (the 
transaction integrity is precisely foreseen to protect you from such cases). 
   
   So while you are quite free to use it as you want (as long as problems 
connected with it will not be unnecessary brought to community here without 
letting people know that you have this custom configuration) - it's a different 
thing if we would explicitly expose the feature of modifying sqlalchemy session 
to all users  - because they will **think** that they can do it lightly and 
bring us problems when they will be modifying it (and often won't even tell us 
what modifications they did).
   
   I think we have to make sure that there are appropriate warnings and proper 
description if that feature is going to be added - specifically stating that 
you have to be really knowledgable modifying this, that you should not do it. 
   
   I even think - specifically warning people against this particular feature 
of retrying the session - because we already know that people **think** they 
can use it and that it willl solve their problems with database stability, whre 
we **know** it will make things worse - and solving stability issue is the only 
solution. This should be clearly described as anti-pattern,
   
   In order to make this mergable, the configuration has to be described and 
documented in 
https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml.schema.json
 and covered by unit tests. And those warnings and disclaimers need to be in.


-- 
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]

Reply via email to