You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/10/04 20:08:15 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #18722: Remove repetitive call to get executor class from config
khalidmammadov opened a new pull request #18722:
URL: https://github.com/apache/airflow/pull/18722
This is code improvement PR and removes call to get executor class from the config.
It will instead call self.executor property to invoke call to read from config and cache.
This also simplifies code.
I also added comment for that assignment as it looks like executor from the arguments only used for tests and not intended to be supplied by user.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] kaxil closed pull request #18722: Remove repetitive call to get executor class from config
Posted by GitBox <gi...@apache.org>.
kaxil closed pull request #18722:
URL: https://github.com/apache/airflow/pull/18722
--
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
[GitHub] [airflow] khalidmammadov commented on pull request #18722: Remove repetitive call to get executor class from config
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18722:
URL: https://github.com/apache/airflow/pull/18722#issuecomment-934547834
I dont think this change will bring that issue back as executor assignment logic remains the same and loading executor will be the still the same. The only difference executor_class will be fetched either from executor which is just after that arguments assignment in the IF. Otherwise, it will be looked up through property.
Is there any test case that checks that scenario? Tests I run all passed.
--
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
[GitHub] [airflow] kaxil commented on pull request #18722: Remove repetitive call to get executor class from config
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #18722:
URL: https://github.com/apache/airflow/pull/18722#issuecomment-934659751
> I dont think this change will bring that issue back as executor assignment logic remains the same and loading executor will be the still the same. The only difference executor_class will be fetched either from executor which is just after that arguments assignment in the IF. Otherwise, it will be looked up through property. Is there any test case that checks that scenario? Tests I run all passed.
As soon as you do `self.executor_class = self.executor.__class__.__name__` it needlessly loads the executor just to get the Class name. Please check detailed description in https://github.com/apache/airflow/pull/16700
--
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
[GitHub] [airflow] kaxil edited a comment on pull request #18722: Remove repetitive call to get executor class from config
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #18722:
URL: https://github.com/apache/airflow/pull/18722#issuecomment-934659751
--
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
[GitHub] [airflow] kaxil edited a comment on pull request #18722: Remove repetitive call to get executor class from config
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #18722:
URL: https://github.com/apache/airflow/pull/18722#issuecomment-934659751
> I dont think this change will bring that issue back as executor assignment logic remains the same and loading executor will be the still the same. The only difference executor_class will be fetched either from executor which is just after that arguments assignment in the IF. Otherwise, it will be looked up through property. Is there any test case that checks that scenario? Tests I run all passed.
As soon as you do `self.executor_class = self.executor.__class__.__name__` (out of if statement) it needlessly loads the executor (check code block below) just to get the Class name. Please check detailed description in https://github.com/apache/airflow/pull/16700
https://github.com/apache/airflow/blob/47f43ec7f32c1887f074ef2dbd88d27aa0c1d684/airflow/jobs/base_job.py#L112
--
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