jdbc-connectors: remove LEGACY state #33201

subodh1810 · 2023-12-06T20:35:30Z

vercel · 2023-12-06T20:35:37Z

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		Dec 13, 2023 0:09am

github-actions · 2023-12-06T20:35:52Z

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.

# Conflicts: # airbyte-integrations/connectors/source-postgres/src/test/java/io/airbyte/integrations/source/postgres/CdcPostgresSourceTest.java # airbyte-integrations/connectors/source-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresSourceTest.java

github-actions · 2023-12-12T14:38:58Z

Coverage report for source-postgres

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

Total Project Coverage	71.69%	🍏

postamar

Sorry for taking a while to review this. There's a lot here! Kudos.

The CDK changes LGTM. Note that there are comments I left in non-CDK files about things which could be in the CDK.
This PR does other things besides removing LEGACY: it bumps the CDK dependency on all java connectors and tries to get the connectors tests to pass, which in some cases hasn't been done in more than a year.
Given that, I suggest you limit this PR to CDK changes only and open separate PRs for each connector. We can then deal with each one independently and on our own schedule, in a way that doesn't involve disabling tests unless we have a good reason to (and then state that reason in the annotation). Those PRs will also be easier to review. I'll be happy to help you do this.

postamar · 2023-12-12T19:21:15Z

...onnectors/source-tidb/src/test/java/io/airbyte/integrations/source/tidb/TiDBSourceTests.java

 import org.junit.jupiter.api.Test;
 import org.testcontainers.containers.GenericContainer;
 import org.testcontainers.utility.DockerImageName;

+@Disabled


Is this deliberate? I don't understand why this test is disabled. This comment applies elsewhere in this PR as well.

postamar · 2023-12-12T19:29:49Z

...nnectors/source-tidb/src/test/java/io/airbyte/integrations/source/tidb/TiDBTestDatabase.java

+
+  }
+
+}


This works. Though, the point of these objects is that many instances of these can share one container. It might be worth adding a // TODO: implement shared container comment or something.

postamar · 2023-12-12T19:32:05Z

.../src/test/java/io/airbyte/cdk/integrations/destination/s3/avro/AvroSerializedBufferTest.java

@@ -57,6 +58,8 @@ public static void setup() {
  }

  @Test
+  @Disabled("Flaky on CI, See run https://github.com/airbytehq/airbyte/actions/runs/7126781640/job/19405426141?pr=33201 " +
+      "org.opentest4j.AssertionFailedError: Expected size between 964 and 985, but actual size was 991 ==> expected: <true> but was: <false>")


Urgh, this again. These tests should be rewritten to use deterministic inputs.

This is out of scope for this PR, naturally.

postamar · 2023-12-12T19:36:14Z

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

+
+  @Override
+  public String getDriverClassName() {
+    return SNOWFLAKE.getDriverClassName();


Why SNOWFLAKE?

postamar · 2023-12-12T19:37:36Z

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

+
+import org.testcontainers.containers.JdbcDatabaseContainer;
+
+public class NonContainer extends JdbcDatabaseContainer<NonContainer> {


Commentary required. Do I understand correctly that this is a shim for when a test relies on a shared instance somewhere rather than a testcontainer?

postamar · 2023-12-12T19:40:26Z

...rce-tidb/src/test/java/io/airbyte/integrations/source/tidb/TiDBJdbcSourceAcceptanceTest.java

-  public String getDriverClass() {
-    return TiDBSource.DRIVER_CLASS;
+  public JsonNode config() {
+    return Jsons.clone(testdb.configBuilder().build());


Cloning the return value of build is not required, it's a fresh JsonNode. This comment applies elsewhere in this PR as well.

postamar · 2023-12-12T19:43:50Z

...rce-teradata/src/test/java/io/airbyte/integrations/source/teradata/TeradataTestDatabase.java

+import java.util.stream.Stream;
+import org.jooq.SQLDialect;
+
+public class TeradataTestDatabase extends TestDatabase<NonContainer, TeradataTestDatabase, TeradataTestDatabase.TeradataDbConfigBuilder> {


Consider defining a NonContainerTestDatabase class in the CDK which extends TestDatabase<NonContainer, NonContainerTestDatabase, NonContainerTestDatabase.NonContainerTestDatabaseConfigBuilder> and use that here and in other connectors which don't rely on testcontainers. Inject the username, password, sql dialect etc. as constructor args.

postamar · 2023-12-12T19:45:36Z

.../connectors/source-db2/src/test/java/io.airbyte.integrations.source.db2/Db2TestDatabase.java

+  public Db2TestDatabase initialized() {
+    if (!containerStarted) {
+      container.start();
+      containerStarted = true;


The .start() method is idempotent so containerStarted isn't needed. Plus, it's static for some reason.

@postamar The start method is called from createTestDatabase within setup which is executed before each unit test, I wanted to use the same connector for all the unit tests and not have docker container start and stop again. In order to do that I had to introduce containerStarted and make it static

@BeforeEach public void setup() throws Exception { customSetup(); testdb = createTestDatabase();

I understand. The place to put the .start() call is in the @BeforeAll static method.

postamar · 2023-12-12T19:46:13Z

airbyte-integrations/connectors/source-bigquery/acceptance-test-config.yml

-      - config_path: "secrets/sat-config.json"
+  #  discovery:
+  #    tests:
+  #      - config_path: "secrets/sat-config.json"


Is this deliberate?

subodh1810 · 2023-12-13T15:02:56Z

@postamar The aim of the work is to stop emitting LEGACY state and in order to do that I need to release these uncertified connectors so that we are sure that no place is emitting LEGACY state with the latest piece of code. I would like to go ahead with the PR as it is. Remember am not changing anything in the certified connectors and they stay the same, the aim of this work is the uncertified connectors.

Given that, I suggest you limit this PR to CDK changes only and open separate PRs for each connector. We can then deal with each one independently and on our own schedule, in a way that doesn't involve disabling tests unless we have a good reason to (and then state that reason in the annotation). Those PRs will also be easier to review. I'll be happy to help you do this.

postamar

I understand your aim. I strongly believe that it's a bad idea to merge a PR like this one because the risk of breaking the build for a certified connector is too high. For a start, right now, source-mysql doesn't even compile. At least make separate PRs for certified connectors, please. It's really not that much extra work and being sure that you're not creating more work for others is worth it.

I don't care much about the uncertified connectors but they should at least compile. This can be ascertained by the "Repository Health Check". I'm OK with you keeping those in this PR if you insist, since those changes are going to be merged bypassing the branch protections in any case.

postamar · 2023-12-13T17:47:36Z

.../connectors/source-db2/src/test/java/io.airbyte.integrations.source.db2/Db2TestDatabase.java

+  public Db2TestDatabase initialized() {
+    if (!containerStarted) {
+      container.start();
+      containerStarted = true;


I understand. The place to put the .start() call is in the @BeforeAll static method.

subodh1810 · 2023-12-14T07:02:12Z

closing in favor of the following PRs
CDK changes : #33434
Postgres : #33437
MySQL : #33436
Mongo : #33438
MSSQL : #33481
Bigquery + Redshift + Snowflake : https://github.com/airbytehq/airbyte/pull/33484/files
Remaining ones : #33485

wip

3a3af30

subodh1810 self-assigned this Dec 6, 2023

subodh1810 and others added 12 commits December 7, 2023 02:06

wip

3df4a67

Automated Commit - Formatting Changes

6308339

tidb

2c278e4

Automated Commit - Formatting Changes

9729057

teradata + oracle

2bbd4d2

fix compilation error

f05fa6b

missed this one

46de5ec

constants should be final

6327ee5

ignore flaky test

02c947b

fix oracle

6e9f1b4

Automated Commit - Formatting Changes

c6be2c4

enable local-cdk for testing

1d059be

subodh1810 and others added 10 commits December 8, 2023 13:31

remove override

546e5c2

Automated Commit - Formatting Changes

00d60b9

disable oracle tests

767fa56

disable tests

985e587

fix source-acceptance-test + disable teradata and TiDB

a82c61a

disable cockroach-db and click house tests

6ddc12e

disable more

9bd8180

format

b6a2607

disable cockroach

f5f13fa

vercel bot deployed to Preview December 12, 2023 08:54 View deployment

subodh1810 added 3 commits December 12, 2023 15:43

fix postgres compilation error

7871808

disable acceptance test bigquery snowflake

59ed601

fix postgres + disable dynamo + format

43e4b0f

Merge branch 'master' into remove-legacy-state-jdbc-connectors

0617b19

vercel bot deployed to Preview December 12, 2023 16:47 View deployment

subodh1810 added 3 commits December 12, 2023 22:23

elastic changes not required, doesnt work with state

f709c9e

sftp source change not required, doesnt use state

e25806f

e2e test source should emit PER-STREAM state as well

909f2fc

subodh1810 requested a review from akashkulk December 12, 2023 19:48

Merge branch 'master' into remove-legacy-state-jdbc-connectors

96ecb2e

vercel bot deployed to Preview December 12, 2023 19:52 View deployment

postamar reviewed Dec 12, 2023

View reviewed changes

disable acceptance test + pass drive name

8329d9a

subodh1810 requested a review from postamar December 13, 2023 15:03

postamar suggested changes Dec 13, 2023

View reviewed changes

subodh1810 closed this Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jdbc-connectors: remove LEGACY state #33201

jdbc-connectors: remove LEGACY state #33201

subodh1810 commented Dec 6, 2023 •

edited

Loading

vercel bot commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023

github-actions bot commented Dec 12, 2023 •

edited

Loading

postamar left a comment

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

subodh1810 Dec 13, 2023

postamar Dec 13, 2023 •

edited

Loading

postamar Dec 12, 2023

subodh1810 commented Dec 13, 2023

postamar left a comment •

edited

Loading

postamar Dec 13, 2023 •

edited

Loading

subodh1810 commented Dec 14, 2023


		import org.testcontainers.containers.JdbcDatabaseContainer;

		public class NonContainer extends JdbcDatabaseContainer<NonContainer> {

jdbc-connectors: remove LEGACY state #33201

jdbc-connectors: remove LEGACY state #33201

Conversation

subodh1810 commented Dec 6, 2023 • edited Loading

vercel bot commented Dec 6, 2023 • edited Loading

github-actions bot commented Dec 6, 2023

Before Merging a Connector Pull Request

github-actions bot commented Dec 12, 2023 • edited Loading

Coverage report for source-postgres

postamar 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

postamar Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subodh1810 commented Dec 13, 2023

postamar left a comment • edited Loading

Choose a reason for hiding this comment

postamar Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

subodh1810 commented Dec 14, 2023

subodh1810 commented Dec 6, 2023 •

edited

Loading

vercel bot commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

postamar Dec 13, 2023 •

edited

Loading

postamar left a comment •

edited

Loading

postamar Dec 13, 2023 •

edited

Loading