Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove repetitive call to get executor class from config #18722

Closed

Conversation

khalidmammadov
Copy link
Contributor

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 for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:Scheduler Scheduler or dag parsing Issues label Oct 4, 2021
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intended, check #16700

@khalidmammadov
Copy link
Contributor Author

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.

@kaxil
Copy link
Member

kaxil commented Oct 5, 2021

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 #16700

if executor:
# Executor here is used for test suites. So they can inject it for testing
self.executor = executor
self.executor_class = self.executor.__class__.__name__

def executor(self):
return ExecutorLoader.get_default_executor()

@kaxil kaxil closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler Scheduler or dag parsing Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants