Migrate airbyte-bootloader to Micronaut #21073

jdpgrailsdev · 2023-01-05T17:55:55Z

What

Convert airbyte-bootloader to use the Micronaut framework
fix Convert airbyte-bootloader to a Micronaut service #13270
fix Annotate Dependency Services Utilized by airbyte-bootloader #12392

How

Convert airbyte-bootloader to a Micronaut application
Move configuration to Micronaut property source
Convert existing classes to singletons/use dependency injection
Add missing secret persistence type to airbyte-workers

Tests

All unit tests pass
Updated unit tests pass
Application builds locally
Application runs locally
Application runs successfully as part of a local Docker deployment
Tested creating and running a sync locally using database bootstraped by airbyte-bootloader

N.B.: This is a preview PR. I still need to integrate this into airbyte-cloud to verify everything works. Once I have done that, this PR will be moved from a draft to ready for review. However, I don't expect it to change very much, so feel free to do a first pass review.

gosusnp

Exciting to see a bootloader on micronaut, we're getting there!

I don't know if you tested the integration with cloud, but looking at some changes on the bootloader constructor, we may run into some issues.

gosusnp · 2023-01-05T18:21:53Z

.env

@@ -39,6 +39,10 @@ BASIC_AUTH_PASSWORD=password
 BASIC_AUTH_PROXY_TIMEOUT=600

 ### DATABASE ###
+# Airbyte Bootloader environment variables
+BOOTLOADER_MIGRATION_BASELINE_VERSION=0.29.0.001
+RUN_DATABASE_MIGRATION_ON_STARTUP=true


Any cases where we do not want to run migrations?

@gosusnp Don't know. This env var exists for some reason, so I suspect it was added for a particular case. We can obviously remove this in the future if we deem that it is not needed.

gosusnp · 2023-01-05T18:23:19Z

airbyte-bootloader/build.gradle

-    implementation libs.temporal.sdk
-    implementation libs.flyway.core
+    testAnnotationProcessor platform(libs.micronaut.bom)
+    testAnnotationProcessor libs.bundles.micronaut.test.annotation.processor


Out of curiosity, what happened to libs.flyway.core?

It got moved up in this block to ensure that we get the correct version.

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

airbyte-bootloader/src/main/resources/application.yml

airbyte-bootloader/src/test/java/io/airbyte/bootloader/ProtocolVersionCheckerTest.java

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

jdpgrailsdev · 2023-01-11T16:31:29Z

@davinchia The PR has been updated with the changes that we discussed earlier:

All application.yaml files have been updated to disable early fail in Hikaricp
The SecretPersistence interface and LocalTestingSecretPersistence class have been updated to decouple initialization from the constructor to make it compatible with dependency injection. The execution of the initialization logic is now performed by the SecretMigrator from the Bootloader.

davinchia · 2023-01-11T21:58:56Z

airbyte-bootloader/src/main/resources/application.yml

+    connection-test-query: SELECT 1
+    connection-timeout: 30000
+    idle-timeout: 600000
+    initialization-fail-timeout: -1   # Disable fail fast checking to avoid issues due to other pods not being started in time


+1 for the comment

davinchia · 2023-01-11T21:59:28Z

...persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretPersistence.java

@@ -18,6 +18,15 @@
 @SuppressWarnings("PMD.MissingOverride")
 public interface SecretPersistence extends ReadOnlySecretPersistence {

+  /**
+   * Performs any initialization prior to utilization of the persistence object. This exists to make


davinchia · 2023-01-11T22:00:20Z

airbyte-config/init/build.gradle


+    implementation 'commons-cli:commons-cli:1.4'


huh, didn't realise this wasn't already in a deps.toml.

davinchia · 2023-01-11T22:01:59Z

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

-  }
-
-  // TODO will be called automatically by the dependency injection framework on object creation
+  @PostConstruct


Curious, what does this do?

@davinchia This tells the DI framework to run this method after all required singletons used by this singleton are created. It's basically a hook to run some additional init logic. See https://docs.oracle.com/javaee/7/api/javax/annotation/PostConstruct.html for more details.

airbyte-server/src/main/resources/application.yml

davinchia · 2023-01-11T22:09:36Z

airbyte-workers/src/main/java/io/airbyte/workers/config/SecretPersistenceBeanFactory.java

@@ -33,6 +34,8 @@ public class SecretPersistenceBeanFactory {
            pattern = "(?i)^(?!google_secret_manager).*")
  @Requires(property = "airbyte.secret.persistence",
            pattern = "(?i)^(?!vault).*")
+  @Requires(property = "airbyte.secret.persistence",
+            pattern = "(?i)^(?!aws_secret_manager).*")


oh my first time seeing this, what does the pattern do here?

If the value does NOT equal the string (case insensitive).

@davinchia I should add that once this is merged, I plan on doing a second pass to clean this up/consolidate this so that we don't have this in multiple places.

sounds good. just from a reader pov, it's not immediately clear why we need the inverse match here.

davinchia · 2023-01-11T22:10:33Z

docker-compose.yaml

@@ -24,14 +24,10 @@ services:
    container_name: airbyte-bootloader
    environment:
      - AIRBYTE_VERSION=${VERSION}
-      - CONFIG_DATABASE_PASSWORD=${CONFIG_DATABASE_PASSWORD:-}


why are we removing these?

They are no longer used. We reference the DATABASE_X env vars only. We could reintroduce the separate ones if necessary.

ah. if these are no longer used, we should also remove them from the EnvConfigs file.

although, maybe a good idea to wait on that since we haven't yet figured out how we want to track all configuration information globally. i.e. a master list of configuration that our micronaut application yamls accept.

no need to action now. thinking out loud with you

@davinchia Agreed. Once everything is converted to Micronaut, there will be a pass to remove EnvConfigs entirely and clean up the environment variables.

and I hope consolidate into one master list for reference :)

davinchia · 2023-01-11T22:21:15Z

...src/main/java/io/airbyte/config/persistence/split_secrets/LocalTestingSecretPersistence.java


-    Exceptions.toRuntime(() -> {
+  @Override
+  public void initialize() throws SQLException {


I understand your previous comment on the test set up now

davinchia · 2023-01-11T22:21:37Z

airbyte-bootloader/src/test/java/io/airbyte/bootloader/ProtocolVersionCheckerTest.java

    setCurrentProtocolRangeRange(V0_0_0, V2_0_0);
-    setTargetProtocolRangeRange(V0_0_0, V2_0_0);
+    final ProtocolVersionChecker protocolVersionChecker =


I appreciate you cleaning up the tests as you go

davinchia

Nice work!

I appreciate the detailed write up, the comments and the minor clean ups here and there.

* Migrate airbyte-bootloader to Micronaut * PR feedback * More PR feedback * Rename variable for clarity * Add properties to support cloud * Formatting * Use default values for env vars * Re-organization to support cloud overrides * Disable conditional logging * More singleton cleanup * test: try CI without fluentbit * Revert "test: try CI without fluentbit" This reverts commit 8fa0f74. * test: enable SSH on EC2 runner * Revert "test: enable SSH on EC2 runner" This reverts commit e4867aa. * Avoid early database connection on startup * Fix compile issues from refactor * Formatting Co-authored-by: perangel <perangel@gmail.com>

jdpgrailsdev requested review from colesnodgrass, gosusnp, benmoriceau and alovew January 5, 2023 17:55

octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker kubernetes labels Jan 5, 2023

jdpgrailsdev force-pushed the jonathan/airbyte-bootloader-micronaut branch from 207aaf9 to 9b05e22 Compare January 5, 2023 18:18

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 18:21 — with GitHub Actions Inactive

gosusnp requested changes Jan 5, 2023

View reviewed changes

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 19:49 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 19:55 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 20:40 — with GitHub Actions Inactive

jdpgrailsdev force-pushed the jonathan/airbyte-bootloader-micronaut branch from 87a9106 to 947d82b Compare January 5, 2023 20:59

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 21:01 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 21:02 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 5, 2023 21:22 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 6, 2023 15:40 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 6, 2023 15:41 — with GitHub Actions Inactive

octavia-squidington-iv removed the kubernetes label Jan 6, 2023

jdpgrailsdev temporarily deployed to more-secrets January 6, 2023 16:19 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 6, 2023 17:20 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 6, 2023 19:20 — with GitHub Actions Inactive

jdpgrailsdev temporarily deployed to more-secrets January 11, 2023 16:17 — with GitHub Actions Inactive

Fix compile issues from refactor

8992df0

jdpgrailsdev temporarily deployed to more-secrets January 11, 2023 16:17 — with GitHub Actions Inactive

Formatting

1c82df0

jdpgrailsdev temporarily deployed to more-secrets January 11, 2023 16:21 — with GitHub Actions Inactive

Merge branch 'master' into jonathan/airbyte-bootloader-micronaut

07c751c

jdpgrailsdev temporarily deployed to more-secrets January 11, 2023 17:31 — with GitHub Actions Inactive

jdpgrailsdev marked this pull request as ready for review January 11, 2023 17:41

jdpgrailsdev requested a review from a team as a code owner January 11, 2023 17:41

davinchia reviewed Jan 11, 2023

View reviewed changes

airbyte-server/src/main/resources/application.yml Show resolved Hide resolved

davinchia reviewed Jan 11, 2023

View reviewed changes

davinchia approved these changes Jan 11, 2023

View reviewed changes

jdpgrailsdev merged commit 7676af5 into master Jan 12, 2023

jdpgrailsdev deleted the jonathan/airbyte-bootloader-micronaut branch January 12, 2023 14:47

pmossman mentioned this pull request Jan 12, 2023

airbyte worker should be configured with the correct secrets manager based on env vars configured #20997

Closed

11 tasks

octavia-squidington-iii mentioned this pull request Jan 12, 2023

Bump Airbyte version from 0.40.27 to 0.40.28 #21359

Merged

evantahler mentioned this pull request Jan 13, 2023

Airbyte Cron really does re-fetch new connector definitions every 30s #21401

Closed

pedroslopez mentioned this pull request Jan 13, 2023

fix loading remote definitions via cron #21410

Merged

mohitreddy1996 mentioned this pull request Jan 17, 2023

airbyte server: ignore aws secret manager value for default secret persistence #21468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate airbyte-bootloader to Micronaut #21073

Migrate airbyte-bootloader to Micronaut #21073

jdpgrailsdev commented Jan 5, 2023 •

edited

Loading

gosusnp left a comment

gosusnp Jan 5, 2023

jdpgrailsdev Jan 5, 2023

gosusnp Jan 5, 2023

jdpgrailsdev Jan 5, 2023

jdpgrailsdev commented Jan 11, 2023

davinchia Jan 11, 2023

davinchia Jan 11, 2023

davinchia Jan 11, 2023

davinchia Jan 11, 2023

jdpgrailsdev Jan 11, 2023 •

edited

Loading

davinchia Jan 11, 2023

jdpgrailsdev Jan 11, 2023

jdpgrailsdev Jan 12, 2023

davinchia Jan 12, 2023

davinchia Jan 11, 2023

jdpgrailsdev Jan 11, 2023

davinchia Jan 11, 2023 •

edited

Loading

davinchia Jan 11, 2023

jdpgrailsdev Jan 12, 2023

davinchia Jan 12, 2023

davinchia Jan 11, 2023

davinchia Jan 11, 2023

davinchia left a comment

Migrate airbyte-bootloader to Micronaut #21073

Migrate airbyte-bootloader to Micronaut #21073

Conversation

jdpgrailsdev commented Jan 5, 2023 • edited Loading

What

How

Recommended reading order

Tests

gosusnp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdpgrailsdev commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdpgrailsdev Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia left a comment

Choose a reason for hiding this comment

jdpgrailsdev commented Jan 5, 2023 •

edited

Loading

jdpgrailsdev Jan 11, 2023 •

edited

Loading

davinchia Jan 11, 2023 •

edited

Loading