You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pavansharma36 (via GitHub)" <gi...@apache.org> on 2024/07/26 12:23:43 UTC
[PR] Added support for additional sql alchemy session args [airflow]
pavansharma36 opened a new pull request, #41048:
URL: https://github.com/apache/airflow/pull/41048
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "iw-pavan (via GitHub)" <gi...@apache.org>.
iw-pavan commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2257653258
@potiuk I understand the risk here.
But since its generic enhancement is it possible to merge this?
PS: will try to implement selective retries based on statement in session.
Thanks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2257665886
I don’t disagree that the way you want to use this sounds bad… but yeah the feature itself is not unreasonable. I wonder if instead of suppying kwargs, it’d be easier if we just `getimport` a generic callable, something like
```python
def _default_session_maker(engine):
return sessionmaker(
autocommit=False,
autoflush=False,
bind=engine,
expire_on_commit=False,
)
session_maker = conf.getimport("database", "sql_alchemy_session_maker") or _default_session_maker
Session = scoped_session(session_maker(engine=engine))
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "iw-pavan (via GitHub)" <gi...@apache.org>.
iw-pavan commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2252658640
With this support we want to implement retryable queries
Add retry session like
`class RetryingSession(Session):
__max_retry_count__ = 3
__retry_messages__ = [
"server closed the connection unexpectedly",
"the database system is shutting down",
"the database system is starting up"
]
def __raise_if_not_retryable(self, ex):
exception_msg = str(ex)
for m in RetryingSession.__retry_messages__:
if m in exception_msg:
logging.warning('Database error is retryable : %s', exception_msg)
return
logging.warning('Database error is not retryable : %s', exception_msg)
raise ex
def execute(
self,
*args,
**kwargs
):
attempts = 0
while True:
try:
attempts += 1
return super(RetryingSession, self).execute(*args, **kwargs)
except OperationalError as ex:
self.__raise_if_not_retryable(ex)
if attempts <= self.__max_retry_count__:
sleep_for = 2 ** (attempts - 1)
logging.error(
"/!\ Database connection error: retrying Strategy => sleeping for {}s"
" and will retry (attempt #{} of {}) \n Detailed query impacted: {}".format(
sleep_for, attempts, self.__max_retry_count__, ex)
)
sleep(sleep_for)
else:
raise`
Then in airflow_local_settings
`session_kwargs = dict(
class_= RetryingSession
)`
And airflow cfg
`sql_alchemy_session_args = airflow_local_settings.session_kwargs`
PS: Is this the way to implement retry in all queries or any other better way?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2264065479
@uranusjr ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #41048:
URL: https://github.com/apache/airflow/pull/41048
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for additional sql alchemy session args [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #41048:
URL: https://github.com/apache/airflow/pull/41048#issuecomment-2257395873
This is a very bad idea IMHO. You should fix your database to not to close the connections and not implement "blanket" retries. This might leave your data inconsistent and if you read carefully the stackoverflow question you will see that this is absolutely not recommended by one of the sqlalchemy creators. Just to explicitly quote it here:
> hi there - SQLAlchemy author here. I've just been pointed to this recipe. I would highly recommend against using a pattern such as the above. "retry" for connectivity should be performed at the top level of a transaction only, and that's what the pool pre_ping feature is for. if you lose a connection in the middle of a transaction, you need to re-run your whole operation. explicit is better than implicit. –
[zzzeek](https://stackoverflow.com/users/34549/zzzeek)
[CommentedOct 19, 2020 at 14:48](https://stackoverflow.com/questions/53287215/retry-failed-sqlalchemy-queries#comment113928088_53300049)
And this is exactly what we do in Airflow - we have a number of cases where we explicity retry a transaction on failure - we even have special decorator for that - when we know we can retry a DB transaction safely - following this advice.
By doing it you might end up with a number of problems - that will be impossible to diagnose and you wil be basically on your own scratching your head when it happens (please don't reach out for maintainers help in this case, because we won't be able to help you - other than telling the same - fix your database stability.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org