airbyte-cron: update connector definitions from remote #16438

pedroslopez · 2022-09-08T14:52:16Z

What

closes https://github.com/airbytehq/airbyte-cloud/issues/2542

Adds functionality to update source and destination definitions from the remote catalog at runtime, on an interval.

This builds on the remote definitions provider introduced in #16018

How

Set up airbyte-cron to connect to the configs db by following in airbyte-workers footsteps
Add a new DefinitionsUpdater task for airbyte-cron scheduled to run every 30 secs
Move definitions update logic to new helper ApplyDefinitionsHelper to re-use between bootloader/cron and standardize between oss/cloud. This will replace cloud's ApplySeedsToDefinitions class.
Use deployment mode env var to determine if we should overwrite everything or be more granular in how the update is performed
Allow using UPDATE_DEFINITIONS_CRON_ENABLED env var to turn on / off these updates

Also fixed a couple of things while in here:

When updating definitions for OSS, fix crash that occurs if there's a non-semver docker image tag in the db

🚨 User Impact 🚨

This will be off by default on OSS, so no impact.

pedroslopez · 2022-09-09T17:23:15Z

...onfig-persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java

+    try {
+      return new AirbyteVersion(latestVersion).checkOnlyPatchVersionIsUpdatedComparedTo(new AirbyteVersion(currentVersion));
+    } catch (final Exception e) {
+      LOGGER.error("Failed to check version: {} vs {}", currentVersion, latestVersion);
+      return false;
+    }


This was causing errors and a bootloader crash when updating definitions if any definition was not a valid semver. This would happen any time you've added a custom dev version for testing, for example.

pedroslopez · 2022-09-09T17:25:49Z

airbyte-config/init/src/main/java/io/airbyte/config/init/ApplyDefinitionsHelper.java

+      // todo (pedroslopez): Logic to apply definitions should be moved outside of the
+      // DatabaseConfigPersistence class and behavior standardized
+      final ConfigPersistence dbConfigPersistence = configRepository.getConfigPersistence();
+      final ConfigPersistence providerConfigPersistence = new DefinitionProviderToConfigPersistenceAdapter(definitionsProvider);
+      dbConfigPersistence.loadData(providerConfigPersistence);


Didn't want to spend much time on this since it wasn't the focus of this PR and I think the logic for applying version updates is going to change anyway, so I'm just calling the existing method.

If someone isn't going to pick this up in the next few weeks, we should create an issue and link it here so we don't forget.

pedroslopez · 2022-09-09T17:28:25Z

airbyte-cron/src/main/java/io/airbyte/cron/config/DatabaseBeanFactory.java

+@Factory
+@Slf4j
+@SuppressWarnings("PMD.AvoidDuplicateLiterals")
+public class DatabaseBeanFactory {


I took heavy inspiration from the airbyte-workers micronaut migration for this, but I'm noticing the code is very very similar. Is there a way to share between the two?

cc @jdpgrailsdev

@pedroslopez Yes...if we move any class that is annotated with Micronaut annotations to a library AND include the proper dependencies so that the annotation processor can find those classes during build time, the class will be automatically picked up by Micronaut at runtime from a library. I have done this in my airbyte-workers PR with the :airbyte-config:config-models module. I would also recommend doing this for the ApplyDefintionsHelper, which can just have the annotations placed directly on it (e.g. @Singleton).

I think we should plan on doing a sweep after we get everything working across the services in OSS.

airbyte-bootloader/src/main/java/io/airbyte/bootloader/BootloaderApp.java

pedroslopez · 2022-09-15T15:04:34Z

Hi @gosusnp / @jdpgrailsdev could I get confirmation that this is ok to merge via a review/approval? I have merged in the latest changes from master that remove the airbyte-worker dependency and my changes here are not affected.

Note that I'm defining my own DatabaseBeanFactory here which is why I don't need to depend on airbyte-worker, but I think de-duplicating this can be tackled as part of the micronaut tech debt epic that was being discussed.

jdpgrailsdev · 2022-09-15T15:54:30Z

@pedroslopez I'm fine with merging this. We will want to do some cleanup later to avoid duplicating the configuration factory bits, but that shouldn't hold this up if we have confirmed that the airbyte-workers bit has been removed. I'll defer to @benmoriceau and @gosusnp to make sure that there isn't something in here that would impact the work they are doing to extract some classes from airbyte-workers for use in this service (I don't believe that this is an issue, but worth double checking).

gosusnp

This introduces a new path for reading protocol versions which is missing some recent changes. I put the fix up in #16796

gosusnp · 2022-09-15T22:13:27Z

...onfig-persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java

+      return new AirbyteVersion(latestVersion).checkOnlyPatchVersionIsUpdatedComparedTo(new AirbyteVersion(currentVersion));
+    } catch (final Exception e) {
+      LOGGER.error("Failed to check version: {} vs {}", currentVersion, latestVersion);
+      return false;


Probably return true might be a better behavior here. Typically, if the version is dev, I wonder if we shouldn't always try to refresh?

…vation * master: (26 commits) supply a source id for schema discovery in connector integration tests (#17662) Source Iterable: Add permission check for stream (#17602) Moving TrackingClientSingleton.initialize into the bean itself (#17631) Handle null workspace IDs in tracking/reporting methods gracefully (#17641) Bump Airbyte version from 0.40.11 to 0.40.12 (#17653) Revert "Do not wait the end of a reset to return an update (#17591)" (#17640) Standardize HttpRequester's url_base and path format (#17524) Create geography_type enum and add geography column in connection and workspace table (#16818) airbyte-cron: update connector definitions from remote (#16438) Do not wait the end of a reset to return an update (#17591) Remove redundant title labels from connector specs (#17544) Updated GA4 status support large schema discovery (#17394) 🪟 🐛 Fixes connector checks not properly ending their loading state (#17620) 🪟🧪 [Experiment] add hideOnboarding experiment (#17605) Source Recharge: change releaseStage to GA (#17606) Source Recharge: skip stream if 403 received (#17608) remove sonar-scan workflow (#17609) Mark/tables should be full width on all pages (#17401) Auto fail all workfow if there is a Versioning issue (#17562) ...

* add db connection injection to airbyte-cron * load definitions * fix patch version check for non-semver tags * apply helper tests * logging updates * fix remote definitions provider: add tombstones * docker compose env updates * add test for tombstone presence * rename helper class * config updates * dont use optionals as fields * use apply helper instead of directly calling load data in bootloader * avoid pmd warn * add docstring * sort docker compose env vars * updates for javax -> jakarta * clean up docker compose, update kube deployment env

pedroslopez added 2 commits September 7, 2022 22:20

add db connection injection to airbyte-cron

b20b56b

load definitions

afcce07

pedroslopez temporarily deployed to more-secrets September 8, 2022 14:54 Inactive

pedroslopez added 5 commits September 9, 2022 01:40

fix patch version check for non-semver tags

2c565ad

apply helper tests

a57c916

logging updates

13e09dd

fix remote definitions provider: add tombstones

e5df0e2

docker compose env updates

c01ce66

pedroslopez temporarily deployed to more-secrets September 9, 2022 05:45 Inactive

pedroslopez added 3 commits September 9, 2022 11:27

add test for tombstone presence

a6d37f2

rename helper class

1d2c318

config updates

cbb44ad

pedroslopez temporarily deployed to more-secrets September 9, 2022 16:18 Inactive

dont use optionals as fields

aabea9b

pedroslopez temporarily deployed to more-secrets September 9, 2022 16:32 Inactive

Merge branch 'master' into pedroslopez/remote-defs-cron

8e03568

pedroslopez temporarily deployed to more-secrets September 9, 2022 17:12 Inactive

use apply helper instead of directly calling load data in bootloader

4090dd5

pedroslopez temporarily deployed to more-secrets September 9, 2022 17:23 Inactive

avoid pmd warn

2ed802c

pedroslopez commented Sep 9, 2022

View reviewed changes

pedroslopez changed the title ~~Pedroslopez/remote defs cron~~ airbyte-cron: update connector definitions from remote Sep 9, 2022

pedroslopez temporarily deployed to more-secrets September 9, 2022 19:32 Inactive

pedroslopez marked this pull request as ready for review September 9, 2022 20:16

pedroslopez requested review from jdpgrailsdev, sherifnada and davinchia September 9, 2022 20:16

Merge branch 'master' into pedroslopez/remote-defs-cron

aa0ad12

pedroslopez temporarily deployed to more-secrets September 14, 2022 23:10 Inactive

pedroslopez added 2 commits September 15, 2022 10:59

add docstring

ed358e1

sort docker compose env vars

ecd10ad

pedroslopez requested a review from gosusnp September 15, 2022 15:04

pedroslopez temporarily deployed to more-secrets September 15, 2022 15:04 Inactive

Merge branch 'master' into pedroslopez/remote-defs-cron

650a08c

pedroslopez temporarily deployed to more-secrets September 15, 2022 15:07 Inactive

gosusnp mentioned this pull request Sep 16, 2022

DefinitionProviders adds default protocol version for connectors #16796

Merged

gosusnp approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'master' into pedroslopez/remote-defs-cron

e6d7ee9

pedroslopez temporarily deployed to more-secrets October 3, 2022 21:51 Inactive

updates for javax -> jakarta

e2f9ca4

pedroslopez temporarily deployed to more-secrets October 5, 2022 19:34 Inactive

pedroslopez added 2 commits October 5, 2022 15:39

Merge branch 'master' into pedroslopez/remote-defs-cron

7dd240f

clean up docker compose, update kube deployment env

aa824e0

github-actions bot added area/platform issues related to the platform kubernetes labels Oct 5, 2022

pedroslopez temporarily deployed to more-secrets October 5, 2022 20:34 Inactive

pedroslopez merged commit 69f53eb into master Oct 5, 2022

pedroslopez deleted the pedroslopez/remote-defs-cron branch October 5, 2022 21:21

This was referenced Oct 5, 2022

Bump Airbyte version from 0.40.11 to 0.40.12 #17635

Closed

Bump Airbyte version from 0.40.11 to 0.40.12 #17653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

airbyte-cron: update connector definitions from remote #16438

airbyte-cron: update connector definitions from remote #16438

pedroslopez commented Sep 8, 2022 •

edited

Loading

pedroslopez Sep 9, 2022

pedroslopez Sep 9, 2022

davinchia Sep 14, 2022

pedroslopez Sep 9, 2022

pedroslopez Sep 9, 2022

jdpgrailsdev Sep 12, 2022 •

edited

Loading

pedroslopez commented Sep 15, 2022

jdpgrailsdev commented Sep 15, 2022

gosusnp left a comment

gosusnp Sep 15, 2022

airbyte-cron: update connector definitions from remote #16438

airbyte-cron: update connector definitions from remote #16438

Conversation

pedroslopez commented Sep 8, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

pedroslopez Sep 9, 2022

Choose a reason for hiding this comment

pedroslopez Sep 9, 2022

Choose a reason for hiding this comment

davinchia Sep 14, 2022

Choose a reason for hiding this comment

pedroslopez Sep 9, 2022

Choose a reason for hiding this comment

pedroslopez Sep 9, 2022

Choose a reason for hiding this comment

jdpgrailsdev Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

pedroslopez commented Sep 15, 2022

jdpgrailsdev commented Sep 15, 2022

gosusnp left a comment

Choose a reason for hiding this comment

gosusnp Sep 15, 2022

Choose a reason for hiding this comment

pedroslopez commented Sep 8, 2022 •

edited

Loading

jdpgrailsdev Sep 12, 2022 •

edited

Loading