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

[AIRFLOW-245] Add access to task instance to executors #1596

Closed
wants to merge 1 commit into from
Closed

[AIRFLOW-245] Add access to task instance to executors #1596

wants to merge 1 commit into from

Conversation

alexandrnikitin
Copy link

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

Testing Done:

Reminders for contributors:

@codecov-io
Copy link

codecov-io commented Jun 15, 2016

Current coverage is 68.03%

Merging #1596 into master will not change coverage

@@             master      #1596   diff @@
==========================================
  Files           116        116          
  Lines          8310       8310          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           5654       5654          
  Misses         2656       2656          
  Partials          0          0          

Powered by Codecov. Last updated by 8aa7160...464fa92

@bolkedebruin
Copy link
Contributor

Please make your commit follow the guidelines (description, commit message etc). In addition please include tests. Thanks!

Motivation: Working on a custom executor I want to utilize the existing ecosystem: all operators e.g. DockerOperator and access its fields like image, command, volumes. I want to  transform/convert them to be used by another execution system like Mesos. So that I add ability to have access to task instances from an executor.

Resolves: AIRFLOW-245
@alexandrnikitin
Copy link
Author

@bolkedebruin Thank you for the reply. I changed the commit. Regarding tests: There's not a single test of any of the executors. I don't know where to start from 😞

@jlowin
Copy link
Member

jlowin commented Jun 20, 2016

Something important to keep in mind -- the current version of execute_async() just requires the task key (a bunch of strings) and the command that runs the task (another string, something like airflow run task). That's because we make no assumptions about how the execution is implemented, or on what machine/environment, or even when it will be run. Passing a task object to the execute_async() method is contrary to that assumption. In particular, there is no guarantee that task objects are serializable.

The key should give enough information to load the task inside the executor by querying the database for the matching taskinstance -- I think that would be the far preferable way to implement the desired behavior in a custom executor.

@alexandrnikitin
Copy link
Author

I close it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants