java CDK, source-postgres: parallelize tests #32314

postamar · 2023-11-08T18:47:39Z

This PR parallelizes the tests and integration tests for source-postgres using junit's parallelism instead of gradle's. This allows us to start singleton test containers which are shared by all tests. Changing the parallelism forces the removal of the @SystemStub annotation which isn't supported in this case; I get around setting env vars by using and extending FeatureFlags.

The changes to the source-postgres tests are superficial in nature. A lot of the diff consists of lines whose indentation levels were changed. The parts of this PR deserving more scrutiny are in airbyte-cdk/**. Note that CI is failing but that's because of this annoying dockerhub rate limit again; airbyte-ci connectors --name=source-postgres test completes successfully in 7 minutes on my laptop.

Fixes #31818.

vercel · 2023-11-08T18:47:45Z

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 8, 2023 10:01pm

github-actions · 2023-11-08T18:47:56Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

github-actions · 2023-11-08T18:55:08Z

Coverage report for source-postgres

File	Coverage [88.9%]	🍏
PostgresSource.java	88.9%	🍏

Total Project Coverage	71.99%	🍏

postamar · 2023-11-08T19:19:50Z

...a/io/airbyte/cdk/integrations/debezium/internals/postgres/PostgresReplicationConnection.java

@@ -48,7 +48,7 @@ public static Connection createConnection(final JsonNode jdbcConfig) throws SQLE
      validateReplicationConnection(connection);
      return connection;
    } catch (final PSQLException exception) {
-      if (exception.getMessage().equals("FATAL: must be superuser or replication role to start walsender")) {
+      if ("42501".equals(exception.getSQLState())) { // insufficient_privilege


The existing logic doesn't work for postgres 16. In any case we shouldn't have business logic depending on textual messages, as far as postgres is concerned the sql state values are the proper API to use.

postamar · 2023-11-08T19:20:40Z

...byte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/PostgresTestDatabase.java

+        "chmod 0600 ~/.postgresql/postgresql.crt ~/.postgresql/postgresql.key",
+        "cp ca.crt root/.postgresql/ca.crt",
+        "chown postgres:postgres ~/.postgresql/ca.crt",
+        "psql -U test -c \"SELECT pg_reload_conf();\"",


These all came straight out of PostgresUtils::getCertificate.

postamar · 2023-11-08T19:21:16Z

airbyte-integrations/connectors/source-postgres/build.gradle

@@ -9,7 +9,7 @@ plugins {
 airbyteJavaConnector {
    cdkVersionRequired = '0.4.1'
    features = ['db-sources']
-    useLocalCdk = false
+    useLocalCdk = true


Reminder to undo this and bump the required CDK version after publishing it.

postamar · 2023-11-08T19:22:03Z

airbyte-integrations/connectors/source-postgres/gradle.properties

@@ -0,0 +1 @@
+testExecutionConcurrency=-1


This triggers the junit test execution concurrency (instead of the default gradle concurrency). What this means is that the tests run in different threads rather than different processes.

Do we tell also gradle to not do concurrency?

Indeed we do, some connectors have testExecutionConcurrency=1 which effectively disables concurrency.

rodireich · 2023-11-08T21:11:32Z

Question: do tests still pass with gradle … test?
Is it going to be faster with gradle?

postamar · 2023-11-08T21:12:17Z

Question: do tests still pass with gradle … test?
Is it going to be faster with gradle?

yes, and yes

stephane-airbyte · 2023-11-08T21:28:56Z

...byte-cdk/db-sources/src/testFixtures/java/io/airbyte/cdk/testutils/PostgresTestDatabase.java

+    } catch (final IOException e) {
+      throw new UncheckedIOException(e);
+    }
+    this.tmpDir.toFile().deleteOnExit();


I don't think it will delete a directory that's not empty but I might be wrong

I think you're right.

rodireich · 2023-11-08T21:35:54Z

...s/src/test/java/io/airbyte/integrations/source/postgres/CdcPostgresSourceLegacyCtidTest.java

-  protected String getServerImageName() {
-    return "debezium/postgres:13-alpine";
+  protected static String getServerImageName() {
+    return "debezium/postgres:12-bullseye";


I'd leave maybe some on 13 in case there's a difference between 12 and 13.

postamar · 2023-11-08T21:45:57Z

Thanks for the review!

postamar · 2023-11-08T21:46:05Z

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/6804374573
✅ Successfully published Java CDK version=0.4.6!

postamar · 2023-11-08T22:29:20Z

/approve-and-merge reason="test-only change"

octavia-approvington · 2023-11-08T22:29:57Z

A crack team of mammals has made a decision.

java CDK, source-postgres: parallelize tests

5004666

postamar requested a review from a team as a code owner November 8, 2023 18:47

octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit labels Nov 8, 2023

octavia-squidington-iii added the connectors/source/postgres label Nov 8, 2023

octavia-squidington-iv requested a review from a team November 8, 2023 18:49

postamar removed the request for review from a team November 8, 2023 19:16

postamar commented Nov 8, 2023

View reviewed changes

fix PSQL_DB initialization in PostgresJdbcSourceAcceptanceTest

aee9061

stephane-airbyte reviewed Nov 8, 2023

View reviewed changes

rodireich reviewed Nov 8, 2023

View reviewed changes

rodireich approved these changes Nov 8, 2023

View reviewed changes

postamar added 3 commits November 8, 2023 16:41

replace deleteOnExit with shutdown hook

89cd378

s/12-bullseye/13-bullseye/ in an integration test

c062625

Merge branch 'master' into source-postgres-test-refactor

8de26ab

vercel bot deployed to Preview November 8, 2023 21:44 View deployment

bump CDK version to 0.4.6 and adopt in source-postgres

bfa04d2

useLocalCdk = false

22c440c

postamar enabled auto-merge (squash) November 8, 2023 22:01

octavia-approvington approved these changes Nov 8, 2023

View reviewed changes

octavia-approvington merged commit 5013f95 into master Nov 8, 2023
16 of 18 checks passed

octavia-approvington deleted the postamar/source-postgres-test-refactor branch November 8, 2023 22:30

postamar mentioned this pull request Nov 9, 2023

java CDK: skip dropping database in PostgresTestDatabase::close #32377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

java CDK, source-postgres: parallelize tests #32314

java CDK, source-postgres: parallelize tests #32314

postamar commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023 •

edited

Loading

postamar Nov 8, 2023

postamar Nov 8, 2023

postamar Nov 8, 2023

postamar Nov 8, 2023

rodireich Nov 8, 2023

postamar Nov 8, 2023

rodireich commented Nov 8, 2023

postamar commented Nov 8, 2023

stephane-airbyte Nov 8, 2023

postamar Nov 8, 2023

rodireich Nov 8, 2023

postamar commented Nov 8, 2023

postamar commented Nov 8, 2023 •

edited by github-actions bot

Loading

postamar commented Nov 8, 2023

octavia-approvington commented Nov 8, 2023

		@@ -0,0 +1 @@
		testExecutionConcurrency=-1

java CDK, source-postgres: parallelize tests #32314

java CDK, source-postgres: parallelize tests #32314

Conversation

postamar commented Nov 8, 2023 • edited Loading

vercel bot commented Nov 8, 2023 • edited Loading

github-actions bot commented Nov 8, 2023

Before Merging a Connector Pull Request

github-actions bot commented Nov 8, 2023 • edited Loading

Coverage report for source-postgres

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

rodireich commented Nov 8, 2023

postamar commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar commented Nov 8, 2023

postamar commented Nov 8, 2023 • edited by github-actions bot Loading

postamar commented Nov 8, 2023

octavia-approvington commented Nov 8, 2023

postamar commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 8, 2023 •

edited

Loading

postamar commented Nov 8, 2023 •

edited by github-actions bot

Loading