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 orchestrator improvements #153

Merged
merged 18 commits into from
Nov 4, 2021
Merged

Conversation

schustmi
Copy link
Contributor

@schustmi schustmi commented Nov 2, 2021

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Describe changes

  • Airflow orchestrator doesn't start by itself now but requires a CLI call (same for shutting down)
  • Added ability to monitor an orchestrator via the CLI

@github-actions github-actions bot added the internal To filter out internal PRs and issues label Nov 2, 2021
@schustmi
Copy link
Contributor Author

schustmi commented Nov 2, 2021

This PR does not update the docs with the new Airflow Orchestrator workflow (yet). I will add it here or in a separate PR once we think the usage is how we want it to be.

Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

Great job!

@@ -234,29 +237,94 @@ def delete_orchestrator(orchestrator_name: str) -> None:
cli_utils.declare(f"Deleted orchestrator: `{orchestrator_name}`")


def _get_orchestrator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps move this to the stack or the service , what ever is more generalizable and makes sense? We should also probably do it for the artifact store and metadata store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I remember why I put it here (both service and stack already have a way to get the active orchestrator or by key): The reason was that I wanted do declare something on the CLI if we fallback to the active orchestrator, so it shouldn't be part of the other classes I think. I'd argue in this case it should probably stay in the cli orchestrator file, wdyt?

@htahir1 htahir1 merged commit 43ca792 into main Nov 4, 2021
@htahir1 htahir1 deleted the michael/ENG-53-airflow-orchestrator branch November 4, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants