🐛 Save Temporal Workflow Id to the DB instead of the Workspace volume.

[davinchia]

What

A few days ago we removed the workflow volume from the Kubernetes deployment in order to simplify the set up. This also lets us do away with the workspace PVC.

In theory this isn't needed since the volume is mainly used for logs and our Kube deployment logs out to the Cloud storage.

In the process of doing so, we realised the volume is used to store the temporal workflow id that is later used to cancel the workflow.

This PR:

1. Adds a migration to add the temporalWorkflowId column to the Attempts table. Exposes various persistent methods for this.
2. Modify temporal to store the workflow id in this column. Modify cancellation to retrieve the workflow id from the table.

Things to call out:

1. This approach means the worker now requires access to the jobs DB. I think this is reasonable.
2. Some tests are disabled since we haven't really stabilised the Flyway + older file-base migrations yet. Follow up ticket has been created (Update config and job persistence unit tests to run migrations #5857) and Liren is working on this.

Recommended reading order

1. Attempts.yaml and V0_29_15_001__Add_temporalWorkflowId_col_to_Attempts.java to understand the schema changes.
2. JobPersistence.java and DefaultJobPersistence.java to understand how the new column is used.
3. TemporalAttemptExecution.java and SchedulerHandler.java to understand how cancellation is happening.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

• Community member? Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • docs/SUMMARY.md
  • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • docs/integrations/README.md
  • airbyte-integrations/builds.md
• PR name follows PR naming conventions
• Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions
• Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

• Issue acceptance criteria met
• PR name follows PR naming conventions
• If adding a new generator, add it to the list of scaffold modules being tested
• The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
• Documentation which references the generator is updated as needed.

[davinchia]

@jrhizor heads up, turns out that this wasn't as straightforward as we thought. What do you think about the approach listed out?

I think it would also close #2564 at the same time. I looked into using Temporal's metadata store and it looks like this requires elastisearch, so using the default workflow id in a DB seems o-kay to me.

[tuliren]

The failed unit test, testJobsOfLastMigrationMatchSource, is no longer relevant. I think you can delete it.

[davinchia]

@tuliren cool I'll delete it. Thanks!

We'll have to do the same for the other test method for the config database.

What's the best way for us to remember to clean this up? Is there an existing ticket or should I write one?

[tuliren]

I created a ticket: #5857

[davinchia]

@tuliren since this is the first time we are doing a Flyway migration, I ran test to confirm backwards compatibility with previous versions and everything seemed to work. I stood up Airbyte on 0.29.15-alpha, performed one sync, spun down the server, then bring it up at version dev. Saw the migration being applied and everything seemed to be working. Makes me think we are ok.

Can you confirm this is alright? Slightly nervous since our migration tests aren't completely working.

[jrhizor]

Looks good. Only minor comments.

Are we only going to merge this after the migration tests are fixed? I'm a bit worried about having disabled migration tests over any releases.

[cgardens]

nice!

[tuliren]

@tuliren since this is the first time we are doing a Flyway migration, I ran test to confirm backwards compatibility with previous versions and everything seemed to work. I stood up Airbyte on 0.29.15-alpha, performed one sync, spun down the server, then bring it up at version dev. Saw the migration being applied and everything seemed to be working. Makes me think we are ok.

Can you confirm this is alright? Slightly nervous since our migration tests are completely working.

Yes, I think this is alright. Also I will add an acceptance test template for Flyway migration. Issue here: https://github.com/airbytehq/airbyte-internal-issues/issues/232 Will work on this when I copy the job attempt state to the configs database. This should make new migration easier to test.

[davinchia]

Big PR review so going to summarise everything.

@jrhizor with Liren's blessing I think it's okay to ship this without the migration tests since this is backwards compatible. As long as there are no additional migrations in the mean time we are ok. Liren is going to work on fixing this soon, so we should be ok.

Remaining bits for both Charles and Jared to weigh in:

• Should getAttemptTemporalWorkflowId return an optional?
• Do we feel okay with the workflow setting a field in the DB? Is there a better way of doing so?

Thank you for bearing with me on this!

[jrhizor]

If Liren's change is going to happen very short term, shouldn't it block this PR? If Liren's change isn't going to be merged very short term, then it doesn't seem safe to block any other change that might have a migration for that period.

[davinchia]

Spoke offline to Jared - merging this is okay since it only blocks schema migrations in the config database for the json blob.

[davinchia]

The Kube test is passing locally. Going to merge.