java-cdk: introduce TestDatabase by postamar · Pull Request #32656 · airbytehq/airbyte

Marius Posta (postamar) · 2023-11-17T20:22:08Z

This PR extracts a new, generic TestDatabase superclass from the the newly-introduced PostgresTestDatabase, and rewrites the CdcSourceTest and JdbcSourceAcceptanceTest base classes using these types.

vercel · 2023-11-17T20:22:14Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 22, 2023 4:28pm

Marius Posta (postamar) · 2023-11-22T16:24:54Z

@@ -1,74 +0,0 @@
-/*


Moved to source-mysql, see #32658.

Marius Posta (postamar) · 2023-11-22T16:25:12Z

-    }
-
-  }
-


Moved to source-postgres, see #32657.

Marius Posta (postamar) · 2023-11-22T16:26:56Z

+                .filter(d -> d.compareTo(CONNECT_TIMEOUT_DEFAULT) >= 0);
+      };
+      return parsedConnectionTimeout.orElse(CONNECT_TIMEOUT_DEFAULT).toMillis();
+    }


Stephane Geneix (@stephane-airbyte) I hope this is less offensive, despite having left in some Optional usages. I agree the previous iteration wasn't readable but I honestly believe that this one is.

The purpose of adding support for these new databases is to speed up the test cases in which a timeout is expected. We don't want to have to wait a whole minute every time.

Marius Posta (postamar) · 2023-11-22T16:27:48Z

+        .put("tunnel_user_password", tunnelMethod.equals(SSH_PASSWORD_AUTH) ? SSH_PASSWORD : "")
+        .put("ssh_key", tunnelMethod.equals(SSH_KEY_AUTH) ? bastion.execInContainer("cat", "var/bastion/id_rsa").getStdout() : "")
+        .build());
+  }


This got lifted out of getTunnelConfig because we can't use the latter with the new TestDatabase.ConfigBuilder objects.

Marius Posta (postamar) · 2023-11-22T16:28:36Z

+  @Override
+  public void close() {
+    stopAndClose();
+  }


Dunno why this wasn't an AutoCloseable in the first place! I leftstopAndClose in place because why not.

Marius Posta (postamar) · 2023-11-22T16:29:49Z

      mySQLContainer.start();
      final Map<String, String> connectionProperties = Map.of(
-          CONNECT_TIMEOUT, "30");
+          CONNECT_TIMEOUT, "5000");


Updated to reflect the addition of MySQL support in getConnectionTimeoutMs. For MySQL the timeout value is expressed in milliseconds.

Marius Posta (postamar) · 2023-11-22T16:30:10Z

+          Map.of("loginTimeout", "5"));
+      assertNotNull(dataSource);
+      assertEquals(HikariDataSource.class, dataSource.getClass());
+      assertEquals(5000, ((HikariDataSource) dataSource).getHikariConfigMXBean().getConnectionTimeout());


Added a case to cover the new MS SQL Server support in getConnectionTimeoutMs.

Marius Posta (postamar) · 2023-11-22T16:31:30Z

                                final CdcTargetPosition<T> targetPosition,
                                final boolean trackSchemaHistory,
                                final Duration firstRecordWaitTime,
+                                final Duration subsequentRecordWaitTime,


Plumbing this value here and all the way into the DebeziumRecordIterator allows us to speed up the tests by relying on a different default value in tests. This change has no impact in production.

Marius Posta (postamar) · 2023-11-22T16:33:58Z

-  /**
-   * In the default case here, we don't know for sure whether the Debezium Engine will produce records
-   * or not. We therefore pass {@link canShortCircuitDebeziumEngine} = false.
-   */


I'm getting rid of the canShortCircuitDebeziumEngine functionality. It's not easily portable and there are easier ways to speed up the tests almost as well. Namely, heartbeats and a shorter subsequentRecordWaitTime.

Marius Posta (postamar) · 2023-11-22T16:34:52Z

 import org.slf4j.LoggerFactory;

-public class FirstRecordWaitTimeUtil {
+public class RecordWaitTimeUtil {


Renamed this class as a consequence of plumbing subsequentRecordWaitTime.

Marius Posta (postamar) · 2023-11-22T16:35:45Z

+import java.util.Optional;
+import org.junit.jupiter.api.Test;
+
+public class RecordWaitTimeUtilTest {


Renamed FirstRecordWaitTimeUtilTest into this as a consequence of plumbing subsequentRecordWaitTime.

Marius Posta (postamar) · 2023-11-22T16:36:23Z


-  @ParameterizedTest
-  @Disabled
-  @ValueSource(strings = {"pgoutput", "wal2json"})


The removal of wal2json touches #31937.

Marius Posta (postamar) · 2023-11-22T16:37:34Z

-      final var slotStateAtTheEnd = getReplicationSlot(database, fullReplicationSlot, plugin, dbName);
-      Assertions.assertEquals(slotStateAfterCommit, slotStateAtTheEnd);
-
-      container.stop();


Removed this part of the test case which is what covered the debezium short-circuiting functionality in maybeReplicationStreamIntervalHasRecords. It also was flaky which is why the test is no longer disabled.

Marius Posta (postamar) · 2023-11-22T16:38:52Z

 import org.slf4j.LoggerFactory;

-public abstract class CdcSourceTest {
+public abstract class CdcSourceTest<S extends Source, T extends TestDatabase<?, T, ?>> {


This class is rewritten so that the only mutable state shared across test methods is protected T testdb,. The test logic itself remains unchanged.

Marius Posta (postamar) · 2023-11-22T16:40:00Z

  }

-  protected void maybeSetShorterConnectionTimeout() {
+  protected void maybeSetShorterConnectionTimeout(final JsonNode config) {


Signature was changed to reflect that of the similar method in the new JdbcSourceTest fixture.

Marius Posta (postamar) · 2023-11-22T16:43:23Z

+@SuppressFBWarnings(
+                    value = {"MS_SHOULD_BE_FINAL"},
+                    justification = "The static variables are updated in subclasses for convenience, and cannot be final.")
+abstract public class JdbcSourceTest<S extends Source, T extends TestDatabase<?, T, ?>> {


This class is JdbcSourceAcceptanceTest but where the only mutable state shared between test methods is protected T testdb;. This is the same idea as for CdcSourceTest but unlike that class, JdbcSourceAcceptanceTest is still used by many non-certified connectors.

Marius Posta (postamar) · 2023-11-22T16:45:38Z

        systemProperty 'junit.jupiter.execution.parallel.config.strategy', 'fixed'
        systemProperty 'junit.jupiter.execution.parallel.config.fixed.parallelism', 1
+        // Order test classes by annotation.
+        systemProperty 'junit.jupiter.testclass.order.default', 'org.junit.jupiter.api.ClassOrderer$OrderAnnotation'


This allows decorating test classes with @Order(...) annotations to help long-running classes get scheduled early on.

Marius Posta (postamar) · 2023-11-22T16:45:54Z

 class AirbyteJavaConnectorExtension {

-    boolean useLocalCdk = true
+    boolean useLocalCdk


I don't think true is a safe default at all.

Marius Posta (postamar) · 2023-11-22T16:48:18Z

+    void setUseLocalCdk(boolean useLocalCdk) {
+        this.useLocalCdk = useLocalCdk
+        addCdkDependencies()
+    }


This is a bit of a hack (but gradle kind of sucks) to remove the need for the explicit addCdkDependencies() call in the connector's build.gradle, since setting useLocalCdk to either true or false is mandatory anyway. We should revisit this when we revamp the CDK development lifecycle.

The other changes to this file in this diff are benign additions or readability improvements.

Marius Posta (postamar) · 2023-11-22T16:50:06Z

This is now ready for review. I added some commentary to help the review process. See companion PRs for application to certified source connectors:

Marius Posta (postamar) · 2023-11-23T20:23:05Z

Closed in favour of Graphite stacked PR #32772.

java-cdk: introduce TestDatabase

ba57f10

Octavia Squidington III (octavia-squidington-iii) added the CDK Connector Development Kit label Nov 17, 2023

Marius Posta added 4 commits November 17, 2023 15:26

fix format

84b8961

test fixes

8e9efc1

remove dbz short-circuiting logic

d6eda52

wip cdcsourcetest refactor

71c65dd

Marius Posta (postamar) mentioned this pull request Nov 21, 2023

refactor postgres docker image and modifiers #32618

Closed

Marius Posta added 2 commits November 21, 2023 21:24

fixup cdk

e55cbbe

Merge branch 'master' into test-database-cdk

decaa85

vercel Bot deployed to Preview November 22, 2023 02:26 View deployment

Marius Posta (postamar) marked this pull request as ready for review November 22, 2023 02:32

This was referenced Nov 22, 2023

source-postgres: adopt CDK 0.5.0 #32657

Closed

source-mysql: adopt CDK 0.5.0 #32658

Closed

source-mssql: adopt CDK 0.5.0 #32660

Closed

Marius Posta and others added 6 commits November 21, 2023 22:06

add commentary

b6094f3

fix test

b622bac

silence spotbugs

8f3879c

Automated Commit - Formatting Changes

d6da51a

update changelog

570730b

rewrite monad salad

b4140cb

Marius Posta (postamar) commented Nov 22, 2023

View reviewed changes

Automated Commit - Formatting Changes

891c7c4

Marius Posta (postamar) commented Nov 22, 2023

View reviewed changes

Marius Posta (postamar) requested review from a team and Stephane Geneix (stephane-airbyte) November 22, 2023 16:50

Marius Posta (postamar) closed this Nov 23, 2023

Uh oh!

Conversation

Marius Posta (postamar) commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marius Posta (postamar) Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marius Posta (postamar) Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marius Posta (postamar) commented Nov 22, 2023

Uh oh!

Marius Posta (postamar) commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Marius Posta (postamar) commented Nov 17, 2023 •

edited

Loading

vercel Bot commented Nov 17, 2023 •

edited

Loading

Marius Posta (postamar) Nov 22, 2023 •

edited

Loading

Marius Posta (postamar) Nov 22, 2023 •

edited

Loading