Taragolis commented on code in PR #33846:
URL: https://github.com/apache/airflow/pull/33846#discussion_r1314804442
##########
airflow/jobs/backfill_job_runner.py:
##########
@@ -656,8 +656,6 @@ def _per_task_process(key, ti: TaskInstance, session):
_per_task_process(key, ti, session)
try:
session.commit()
- # break the retry loop
- break
Review Comment:
First of all I'm not against rule "use `else` in `try`..`except`", we just
need to use in places where it actually need, rather than always. IMO this is
informal rule, in category "might/might not" and not in "should/should not" and
definitely not in "must".
I'm sure that we have a rule readability first and in previous version it
very easy to see what is going on:
1. Try to commit
2. If success exit from loop
3. If OperationError happen ...
In new version
1. Try to commit
2. If OperationError happen ...
3. (in previous series of `try..except tv series` 🤣 ) if no exception happen
then exit from loop
Someone could also appeal to the fact that `else` in `try...except` would
cost additional ... couple nanoseconds (OMG in Application which communicate
with DB right after commit 🤣 ), and we could easily start to play code golf and
optimise in place where it not required.
##########
airflow/jobs/backfill_job_runner.py:
##########
@@ -656,8 +656,6 @@ def _per_task_process(key, ti: TaskInstance, session):
_per_task_process(key, ti, session)
try:
session.commit()
- # break the retry loop
- break
Review Comment:
First of all I'm not against rule "use `else` in `try`..`except`", we just
need to use in places where it actually need, rather than always. IMO this is
informal rule, in category "might/might not" and not in "should/should not" and
definitely not in "must".
I'm sure that we have a rule readability first and in previous version it
very easy to see what is going on:
1. Try to commit
2. If success exit from loop
3. If OperationError happen ...
In new version
1. Try to commit
2. If OperationError happen ...
3. (in previous series of `try..except tv series` 🤣 ) if no exception happen
then exit from loop
Someone could also appeal to the fact that `else` in `try...except` would
cost additional ... couple nanoseconds (OMG in Application which communicate
with DB right after commit 🤣 ), and we could easily start to play code golf and
optimise in place where it not required.
--
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]
