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.

Sign up for GitHub

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

Jump to bottom

add container orchestrator for normalization #9020

Merged
merged 15 commits into from
Jan 1, 2022
Merged

add container orchestrator for normalization #9020

merged 15 commits into from
Jan 1, 2022

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Dec 21, 2021 •
edited

  • adds the launching for a container orchestrator for normalization
  • refactors to share code better between the replication & normalization orchestrators

Note that this is still feature flagged out. I tested locally with it enabled and it was working on the last commit.

Probably easiest to read in the following order:

  1. the *LauncherWorker classes
  2. JobOrchestrator
  3. ContainerOrchestratorApp
  4. ReplicationJobOrchestrator
  5. NormalizationJobOrchestrator / NormalizationActivityImpl / WorkerApp

closes #8459

@github-actions github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Dec 21, 2021
jrhizor
jrhizor commented Dec 21, 2021
View reviewed changes
docker-compose.yaml Outdated Show resolved Hide resolved
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 21:50 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:04 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:37 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:46 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:49 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:53 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:54 Inactive
@jrhizor jrhizor marked this pull request as ready for review December 24, 2021 22:56
@jrhizor jrhizor temporarily deployed to more-secrets December 24, 2021 22:59 Inactive
@jrhizor jrhizor mentioned this pull request Dec 24, 2021
Merged
davinchia
davinchia reviewed Dec 27, 2021
View reviewed changes
airbyte-workers/src/main/java/io/airbyte/workers/temporal/sync/NormalizationLauncherWorker.java

// we want to filter down to remove secrets, so we aren't writing over a bunch of unnecessary
// secrets
final Map<String, String> envMap = System.getenv().entrySet().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

just a thought - this seems like something we want to enforce in the abstraction to prevent someone from forgetting.

davinchia
davinchia reviewed Dec 27, 2021
View reviewed changes
...orchestrator/src/main/java/io/airbyte/container_orchestrator/ReplicationJobOrchestrator.java

@Override
public void runJob() throws Exception {
final JobRunConfig jobRunConfig = readJobRunConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

to make sure I understand - how does this interact with the SyncWorkflowImpl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the sync workflow calls an orchestrator launcher activity, which launches a pod/container running the orchestrator itself (the ContainerOrchestratorApp which runs theReplicationJobOrchestrator.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, so the control flow = user triggers job -> temporal sync workflow picks it up -> pod created running ContainerOrchestratorApp -> runs replication job -> this creates the source and destination pods. is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

davinchia
davinchia approved these changes Dec 27, 2021
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Generally made sense.

Left some minor comments. Definitely think we can do with better doc strings.

I had one question about how this interacts with the current SyncWorkflowImpl to make sure I understand.

@jrhizor jrhizor temporarily deployed to more-secrets December 30, 2021 02:01 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets January 1, 2022 01:55 Inactive
@jrhizor jrhizor merged commit a36a860 into master Jan 1, 2022
@jrhizor jrhizor deleted the jrhizor/norm-and-dbt-co branch January 1, 2022 02:28
@jrhizor jrhizor mentioned this pull request Jan 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davinchia davinchia davinchia approved these changes

@subodh1810 subodh1810 Awaiting requested review from subodh1810

Assignees
No one assigned
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make normalization use runner container
2 participants
@jrhizor @davinchia