potiuk edited a comment on pull request #18453: URL: https://github.com/apache/airflow/pull/18453#issuecomment-925696400
Just one comment on that one to ask about some common approach here (and maybe an opportunity to get some useful knowledge sharing). I actually put that commit() deliberately there. I know it is not needed, simply for many years when dealing with the databases, I am used to set some explicit boundaries of database transactions (+ the provide_session kind of decorators which are there to pass an open session/transaction down the stack without closing it). So even if the operation does "nothing" between session.open() an exiting the scope of that session I usually mark it es explicit commit(). Especially when there are different branches you can take - and "return" like in this case (it could also be rollback() in this case, to clearly mark where the session is "closed"). The benefit of that approach is that it is more future-proof. It's very easy to imagine (in general case) that someone in the future will add some modification of the database between session and exiting the function and might not notice that there is branch that might lead to "no-commit". Maybe that's not likely in this particular case, but for me it's more of "natural-habit". Whenever I see such situation that there is a branching and "commit()" in one branch and "nothing" in the other, it feels wrong - and I do not have to exercise extra mental power to understand whether this particular case is justified exception or not. For me it is pretty "natural" thing to do: open sessions() -> branch -> make sure the session is treated the same in all branches.. I'd love to hear your thoughts about it :) -- 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]
