Refactor database initialization logic #12961

jdpgrailsdev · 2022-05-18T15:28:22Z

What

Create Database Initialization Listener Interface #12377
Refactor the database initialization/availability logic in preparation for the move to a dependency injection framework.

How

Decouples the database availability, migration and initialization operations from the database connection classes. This is to make it easier to turn the database connection objects into singletons to be managed by a dependency injection framework, while still maintaining the ability to perform those pre-startup actions within the framework. Ultimately, the new classes in this PR will replace the existing logic and will be injected into a startup listener which will guarantee the execution of the checks prior to allowing the service to handle requests/perform operations.
In addition to the refactor, the PR also performs some cleanup on the logic copied to the new checks. In particular, the decision has been made to remove the hasTable check from the code that verifies the database is available and the migration has been performed. The logic behind this decision is that if the migration has been completed successfully, then the tables that the current code verifies would be present, making this check redundant.
For now, the code that initializes the schema if the database has not yet been created has been maintained in the refactored code. There is an open issue to remove this/convert this logic into a migration script, so that the migration process handles ALL of the database object creation, not just some of it.
Mark the MinimumFlywayMigrationVersionCheck and Database classes/methods as deprecated and to be removed in the future once the conversion to the new implementation has been completed.

More Details

To explain what is changing/moving to the new approach, here are some diagrams of the current code and the proposed code:

Existing Database Availability/Migration/Initialization Logic

Proposed Database Availability/Migration/Initialization Logic

One of the key differences, besides being more dependency-injection friendly, is that the proposed change in this PR re-uses the availability check logic, instead of duplicating it. In the current code, the check to verify that the database is available and accepting connections is currently duplicated between the MinimumFlywayMigrationVersionCheck and Database classes.

🚨 User Impact 🚨

There is no user impact as a result of this PR, as the code being added is not currently being utilized. That will occur in a future PR.

Tests

Unit

Unit tests have been added for all of the new classes, with 100% instruction and branch coverage.

Integration

The new classes were added to both the airbyte-bootloader and airbyte-server and tested locally to validate that the services still performed the required checks and could start successful/perform their operations successfully when using the new logic.

benmoriceau

Overall looks good to me, I wonder if we can leverage lombok a little bit more to make the POJO less verbose.

benmoriceau · 2022-05-18T17:19:08Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/DatabaseAvailabilityCheck.java

+
+    while (!initialized) {
+      getLogger().warn("Waiting for database to become available...");
+      if (totalTime >= getTimeoutMs()) {


Should we use a library for retrying (like https://resilience4j.readme.io/docs/retry). Instead of a custom retry policy?

@benmoriceau Definitely. However, for this pass, I am trying to simply copy/move the existing logic to the new classes to make it easier to verify like behavior. Once we get this in place, we can do follow up work to improve this logic.

I would most likely push us to use Failsafe instead of Resilience4j, mainly because it is a bit more modern and is supported by the OpenAPI generation, should we decide to turn it on there too.

benmoriceau · 2022-05-18T17:20:01Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/DatabaseMigrationCheck.java

+   */
+  default void check() throws InterruptedException {
+    final var startTime = System.currentTimeMillis();
+    final var sleepTime = getTimeoutMs() / NUM_POLL_TIMES;


Same comment about the use of the library for retrying

benmoriceau · 2022-05-18T17:21:58Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheck.java

+ */
+public class ConfigsDatabaseAvailabilityCheck implements DatabaseAvailabilityCheck {
+
+  private static final Logger LOGGER = LoggerFactory.getLogger(ConfigsDatabaseAvailabilityCheck.class);


Nit, we can use the @slf4j instead of manually instantiate the logger.

benmoriceau · 2022-05-18T17:22:34Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheck.java

+  // TODO inject via dependency injection framework
+  private final long timeoutMs;
+
+  public ConfigsDatabaseAvailabilityCheck(final DSLContext dslContext, final long timeoutMs) {


nit: we can use @AllArgConstructor to generate this constructor

@benmoriceau This constructor is most likely temporary and will be removed when we add dependency injection. At that point, we can just annotate the member fields with the injection information and can remove the constructor entirely.

benmoriceau · 2022-05-18T17:27:04Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheck.java

+    return Optional.ofNullable(dslContext);
+  }
+
+  @Override


This can be generated with a @Getter annotation

benmoriceau · 2022-05-18T17:27:05Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheck.java

+
+  @Override
+  public Optional<DSLContext> getDslContext() {
+    return Optional.ofNullable(dslContext);


Why do we allow a to have a null dslContext? it should be either mocked or a real context? Could we use something like https://projectlombok.org/features/NonNull to validate it in the constructor? This can be compatible with the constructor annotation as well: https://projectlombok.org/features/constructor

We shouldn't. However, when we move to dependency injection, there is no guarantee that the DSLContext bean will exist. Therefore, I am trying to be defensive here so we can raise a meaningful exception that clearly states what the issue is vs just throwing a NullPointerException and not being obvious.

We shouldn't. However, when we move to dependency injection, there is no guarantee that the DSLContext bean will exist.

This seems like it would be a recurring problem in many places, right? Does this mean we will need to wrap every bean in an optional and have explicit logic that checks if it exists and throws an error if not?

It seems like this is something that the framework should be able to enforce, i.e. bean X is required, and a meaningful exception should be thrown automatically if it does not exist at runtime. Maybe I am misunderstanding something here?

benmoriceau · 2022-05-18T17:28:09Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseMigrationCheck.java

+/**
+ * Implementation of the {@link DatabaseMigrationCheck} for the Configurations database.
+ */
+public class ConfigsDatabaseMigrationCheck implements DatabaseMigrationCheck {


I have the same comment about leveraging lombok to make this class less verbose. Let me know if this would break the compatibility with micronaut.

benmoriceau · 2022-05-18T17:30:33Z

airbyte-db/lib/src/test/java/io/airbyte/db/check/impl/ConfigsDatabaseMigrationCheckTest.java

+
+  @Test
+  void testMigrationCheck() {
+    final var minimumVersion = "1.0.0";


nit: we can use val instead of final var

jdpgrailsdev · 2022-05-18T18:15:33Z

@benmoriceau I think using Lombok is a good suggestion, but I would want to wait until we convert these classes to be dependency injected. I have seen some issues with Lombok-based classes and dependency injection (too much magic being combined), so I am a bit hesitant to introduce Lombok annotations until we get to the DI work and can verify that everything works together.

I should have added in the description that there will be a lot of duplication in the implementations. This is because we will need to inject different versions of the same types into the two different implementations. The only way to do that is to use qualifiers on the injection identified fields to tell the DI framework which version of a bean to inject. Once we get there, we can do another pass to consolidate as much code as possible.

lmossman · 2022-05-19T22:19:27Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/DatabaseAvailabilityCheck.java

+   * The number of times to check if the database is available. TODO replace with a default value in a
+   * value injection annotation
+   */
+  int NUM_POLL_TIMES = 10;


This seems like a constant, and therefore should be final (maybe not super important given the comment above saying that this will be replaced)

This is a constant in an interface, so marking it final doesn't do anything.

Whoops, excuse my tunnel vision here 🤦

airbyte-db/lib/src/main/java/io/airbyte/db/check/DatabaseAvailabilityCheck.java

lmossman · 2022-05-19T22:31:38Z

airbyte-db/lib/src/main/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheck.java

+
+  @Override
+  public Optional<DSLContext> getDslContext() {
+    return Optional.ofNullable(dslContext);


We shouldn't. However, when we move to dependency injection, there is no guarantee that the DSLContext bean will exist.

This seems like it would be a recurring problem in many places, right? Does this mean we will need to wrap every bean in an optional and have explicit logic that checks if it exists and throws an error if not?

It seems like this is something that the framework should be able to enforce, i.e. bean X is required, and a meaningful exception should be thrown automatically if it does not exist at runtime. Maybe I am misunderstanding something here?

lmossman · 2022-05-19T22:35:28Z

airbyte-db/lib/src/main/java/io/airbyte/db/init/DatabaseInitializer.java

+    if (availabilityCheck.isPresent()) {
+      availabilityCheck.get().check();
+      final Optional<DSLContext> dslContext = getDslContext();
+      if (dslContext.isPresent()) {


Related to my above comment - it feels fairly messy to have all of these isPresent() checks, requiring us to nest the actual logic of the method multiple levels deep. Is there a path forward for removing the need for this?

This is mainly a temporary fix, as we don't control the creation of these objects and they can therefore be null. Once we convert to a dependency injection framework, the application will fail to start if a bean of the requested type/name cannot be found (unless of course we mark an injection point as optional).

The alternative in the current code is to put a bunch of != null checks, which are just as messy and not the current, preferred way in Java to handle the potential for an object to not be present/set. If we didn't care about the exceptions being raised, we would use .ifPresent(o -> ...) to remove the if checks.

Got it, I think I had misunderstood this comment as saying that we would need to keep these checks even after we fully convert to dependency injection. I'm okay with this as a temporary fix

lmossman · 2022-05-19T22:37:20Z

airbyte-db/lib/src/main/java/io/airbyte/db/init/DatabaseInitializer.java

+  default Collection<String> getTableNames() {
+    return List.of();
+  }


Is it necessary to have this default implementation? It seems like this could lead to issues if someone implements this class without overriding this method, causing the initialize logic to basically be a no-op

This is more temporary code to avoid the possibility that the implementation doesn't set the list or override it. The alternative would be to return an Optional<Collection<String>> to force the implementation to return an empty optional and obviously do the required ifPresent checks.

I might still be misunderstanding something here, but it seems to me that we would expect everything implementing this interface to have some set of tables to use for checking availability.

And if we leave this method unimplemented in the interface, isn't every implementing class required to implement it, or else Java throws a compilation error? It seems like that would enforce this

airbyte-db/lib/src/main/java/io/airbyte/db/check/DatabaseAvailabilityCheck.java

airbyte-db/lib/src/test/java/io/airbyte/db/check/impl/ConfigsDatabaseAvailabilityCheckTest.java

lmossman

Still one open question about the default implementation, but I don't think it is blocking.

lmossman · 2022-05-21T00:58:29Z

airbyte-db/lib/src/main/java/io/airbyte/db/init/DatabaseInitializer.java

+  default Collection<String> getTableNames() {
+    return List.of();
+  }


I might still be misunderstanding something here, but it seems to me that we would expect everything implementing this interface to have some set of tables to use for checking availability.

And if we leave this method unimplemented in the interface, isn't every implementing class required to implement it, or else Java throws a compilation error? It seems like that would enforce this

jdpgrailsdev · 2022-05-23T13:59:40Z

Still one open question about the default implementation, but I don't think it is blocking.

@lmossman You are correct. The trade-off here is forcing the subclasses to implement it combined with null/optional checking VS having a default implementation. As you pointed out correctly, that does shadow the fact that the subclass may forget to implement it. I will convert this to use Optional like the other methods (and the required checks).

salima-airbyte · 2022-09-14T18:51:12Z

Might be impacted by Micronaut migration. To be revisited in the future.

Refactor database initialization logic

58bbb11

jdpgrailsdev requested review from benmoriceau, lmossman and alovew May 18, 2022 15:28

benmoriceau reviewed May 18, 2022

View reviewed changes

Formatting

edb8f7f

jdpgrailsdev temporarily deployed to more-secrets May 18, 2022 18:21 Inactive

Move database constants

9cce6f2

jdpgrailsdev temporarily deployed to more-secrets May 19, 2022 19:10 Inactive

jdpgrailsdev temporarily deployed to more-secrets May 19, 2022 19:11 Inactive

lmossman reviewed May 19, 2022

View reviewed changes

PR feedback

18f0d7a

jdpgrailsdev temporarily deployed to more-secrets May 20, 2022 13:50 Inactive

PR feedback (use custom exceptions)

9cd38b0

jdpgrailsdev temporarily deployed to more-secrets May 20, 2022 15:51 Inactive

lmossman approved these changes May 21, 2022

View reviewed changes

PR feedback

16bac65

jdpgrailsdev temporarily deployed to more-secrets May 23, 2022 14:50 Inactive

Formatting

b3fb5d6

jdpgrailsdev temporarily deployed to more-secrets May 23, 2022 14:58 Inactive

jdpgrailsdev temporarily deployed to more-secrets May 23, 2022 15:26 Inactive

jdpgrailsdev merged commit 9d99688 into master May 23, 2022

jdpgrailsdev deleted the jonathan/database-initialization-listener branch May 23, 2022 16:23

octavia-squidington-iii mentioned this pull request May 25, 2022

Bump Airbyte version from 0.39.1-alpha to 0.39.2-alpha #13188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor database initialization logic #12961

Refactor database initialization logic #12961

jdpgrailsdev commented May 18, 2022 •

edited

Loading

benmoriceau left a comment

benmoriceau May 18, 2022

jdpgrailsdev May 18, 2022

jdpgrailsdev May 19, 2022

benmoriceau May 18, 2022

benmoriceau May 18, 2022

benmoriceau May 18, 2022

jdpgrailsdev May 18, 2022

benmoriceau May 18, 2022

benmoriceau May 18, 2022

jdpgrailsdev May 18, 2022

lmossman May 19, 2022

benmoriceau May 18, 2022

benmoriceau May 18, 2022

jdpgrailsdev commented May 18, 2022 •

edited

Loading

lmossman May 19, 2022

jdpgrailsdev May 20, 2022

lmossman May 21, 2022

lmossman May 19, 2022

lmossman May 19, 2022

jdpgrailsdev May 20, 2022

lmossman May 21, 2022

lmossman May 19, 2022

jdpgrailsdev May 20, 2022 •

edited

Loading

lmossman May 21, 2022

lmossman left a comment

lmossman May 21, 2022

jdpgrailsdev commented May 23, 2022 •

edited

Loading

salima-airbyte commented Sep 14, 2022

Refactor database initialization logic #12961

Refactor database initialization logic #12961

Conversation

jdpgrailsdev commented May 18, 2022 • edited Loading

What

How

More Details

Existing Database Availability/Migration/Initialization Logic

Proposed Database Availability/Migration/Initialization Logic

Recommended reading order

🚨 User Impact 🚨

Tests

benmoriceau 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdpgrailsdev commented May 18, 2022 • 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

Choose a reason for hiding this comment

jdpgrailsdev May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdpgrailsdev commented May 23, 2022 • edited Loading

salima-airbyte commented Sep 14, 2022

jdpgrailsdev commented May 18, 2022 •

edited

Loading

jdpgrailsdev commented May 18, 2022 •

edited

Loading

jdpgrailsdev May 20, 2022 •

edited

Loading

jdpgrailsdev commented May 23, 2022 •

edited

Loading