🎉 Abstract level for SQL relational database sources #4123

DoNotPanicUA · 2021-06-15T08:32:49Z

What

Provide a common abstract class for non-JDBC source implementation.

How

Move all non-JDBC-specific stuff to a superclass.

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

yaroslav-hrytsaienko

please do minor fixes and move it forward

yaroslav-hrytsaienko · 2021-06-22T09:54:27Z

...irbyte/integrations/io/airbyte/integration_tests/sources/ClickHouseSourceAcceptanceTest.java

@@ -126,10 +126,10 @@ protected void setupEnvironment(TestDestinationEnv environment) throws Exception
            config.get("database").asText()),
        ClickHouseSource.DRIVER_CLASS);

-    final String table1 = JdbcUtils.getFullyQualifiedTableName(SCHEMA_NAME, STREAM_NAME);
+    final String table1 = JdbcSourceUtils.getFullyQualifiedTableName(SCHEMA_NAME, STREAM_NAME);


JdbcSourceUtils -> SourceJdbcUtils

yaroslav-hrytsaienko · 2021-06-22T09:54:44Z

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

@@ -38,7 +36,7 @@ dependencies {
    testFixturesImplementation project(':airbyte-protocol:models')
    testFixturesImplementation project(':airbyte-db')
    testFixturesImplementation project(':airbyte-integrations:bases:base-java')
-    testFixturesImplementation project(':airbyte-integrations:connectors:source-jdbc')
+//    testFixturesImplementation project(':airbyte-integrations:connectors:source-jdbc')


yaroslav-hrytsaienko · 2021-06-22T09:57:01Z

...ectors/source-jdbc/src/main/java/io/airbyte/integrations/source/jdbc/AbstractJdbcSource.java

-        throw new RuntimeException(e);
+  @Override
+  protected Map<String, List<String>> discoverPrimaryKeys(JdbcDatabase database,
+                                                          List<TableInfo<CommonField<JDBCType>>> tableInfos) {


please add some info logging

…ource)

s not an abstract class. + add default implementation for `AbstractRelationalDbSource.getFullyQualifiedTableName`

subodh1810 · 2021-06-23T15:50:20Z

Reviewing this

…act-source # Conflicts: # airbyte-integrations/connectors/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSource.java

subodh1810

My thoughts

I am not sure around the usage of interfaces vs abstract classes. I think we can have
I dont understand the differentiation between AbstractRelationalDbSource and AbstractJdbcSource.

Can you run the integration tests for all the sources that this change is impacting

subodh1810 · 2021-06-25T11:31:12Z

airbyte-db/src/main/java/io/airbyte/db/SqlDatabase.java

+import java.sql.SQLException;
+import java.util.stream.Stream;
+
+public abstract class SqlDatabase implements AutoCloseable {


Why is this an abstract class? Why not an interface?

It's a result of the decision to use setters for configs and private variables.

subodh1810 · 2021-06-25T11:33:20Z

airbyte-db/src/main/java/io/airbyte/db/SqlDatabase.java

+
+public abstract class SqlDatabase implements AutoCloseable {
+
+  private JsonNode sourceConfig;


I think sourceConfig and databaseConfig should be final plus should be initialised in a constructor and not via setter methods

The reason why I used setter instead of the obvious constructor here - it's massive signature changes in the existing classes.
I agree that it's technical depth and should be improved in the future. But I decided that it will be too much for this change.
What do you think about a new issue for that?

subodh1810 · 2021-06-25T11:34:13Z

airbyte-db/src/main/java/io/airbyte/db/jdbc/JdbcDatabase.java

@@ -36,21 +39,22 @@
 /**
 * Database object for interacting with a JDBC connection.
 */
-public interface JdbcDatabase extends AutoCloseable {
+public abstract class JdbcDatabase extends SqlDatabase {


Why change to an abstract class?

The superclass SqlDatabase is an abstract class. If we decide to have an abstract class there, we can't use the interface here.

DoNotPanicUA · 2021-06-30T16:06:09Z

/test connector=source-oracle

🕑 source-oracle https://github.com/airbytehq/airbyte/actions/runs/986980860
❌ source-oracle https://github.com/airbytehq/airbyte/actions/runs/986980860

DoNotPanicUA · 2021-06-30T16:06:19Z

/test connector=destination-snowflake

🕑 destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/986981345
❌ destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/986981345

DoNotPanicUA · 2021-06-30T22:50:14Z

/test connector=source-oracle

🕑 source-oracle https://github.com/airbytehq/airbyte/actions/runs/988040712
✅ source-oracle https://github.com/airbytehq/airbyte/actions/runs/988040712

…act-source

DoNotPanicUA · 2021-07-02T15:11:16Z

/test connector=source-cockroachdb

🕑 source-cockroachdb https://github.com/airbytehq/airbyte/actions/runs/994041119
✅ source-cockroachdb https://github.com/airbytehq/airbyte/actions/runs/994041119

subodh1810

I approve but I still think that using setters for

  private JsonNode sourceConfig;
  private JsonNode databaseConfig;

in SqlDatabase is not a good idea. They should be final and initialised in constructors.

Please make sure to test this thoroughly before merging/deploying.

DoNotPanicUA · 2021-07-05T12:15:33Z

/test connector=source-oracle

🕑 source-oracle https://github.com/airbytehq/airbyte/actions/runs/1001067971
✅ source-oracle https://github.com/airbytehq/airbyte/actions/runs/1001067971

DoNotPanicUA · 2021-07-05T12:16:25Z

/test connector=source-clickhouse

🕑 source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/1001070441
✅ source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/1001070441

DoNotPanicUA · 2021-07-05T12:16:40Z

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/1001071413
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/1001071413

DoNotPanicUA · 2021-07-05T12:16:53Z

/test connector=source-jdbc

🕑 source-jdbc https://github.com/airbytehq/airbyte/actions/runs/1001072023
✅ source-jdbc https://github.com/airbytehq/airbyte/actions/runs/1001072023

DoNotPanicUA · 2021-07-05T12:16:59Z

/test connector=source-mysql

🕑 source-mysql https://github.com/airbytehq/airbyte/actions/runs/1001072486
✅ source-mysql https://github.com/airbytehq/airbyte/actions/runs/1001072486

…act-source

DoNotPanicUA · 2021-07-05T12:21:16Z

/test connector=source-db2

🕑 source-db2 https://github.com/airbytehq/airbyte/actions/runs/1001085196
✅ source-db2 https://github.com/airbytehq/airbyte/actions/runs/1001085196

DoNotPanicUA · 2021-07-05T12:21:29Z

/test connector=source-mssql

🕑 source-mssql https://github.com/airbytehq/airbyte/actions/runs/1001085792
❌ source-mssql https://github.com/airbytehq/airbyte/actions/runs/1001085792

DoNotPanicUA · 2021-07-05T12:22:41Z

/test connector=source-redshift

🕑 source-redshift https://github.com/airbytehq/airbyte/actions/runs/1001089267
❌ source-redshift https://github.com/airbytehq/airbyte/actions/runs/1001089267

DoNotPanicUA · 2021-07-05T13:14:21Z

/test connector=source-mssql

clock2 source-mssql https://github.com/airbytehq/airbyte/actions/runs/1001085792
x source-mssql https://github.com/airbytehq/airbyte/actions/runs/1001085792

This error is in the master state. Reproduced at master:
https://github.com/airbytehq/airbyte/actions/runs/1001189702

DoNotPanicUA · 2021-07-05T14:17:40Z

I approve but I still think that using setters for
  private JsonNode sourceConfig;
  private JsonNode databaseConfig;
in SqlDatabase is not a good idea. They should be final and initialised in constructors.

Please make sure to test this thoroughly before merging/deploying.

The corresponding ticket is created #4547. I will take this ticket into the work after the BigQuery source.

DoNotPanicUA requested a review from yaroslav-hrytsaienko June 15, 2021 08:32

github-actions bot added area/connectors Connector related issues area/protocol labels Jun 15, 2021

DoNotPanicUA force-pushed the aleonets/4024-abstract-source branch from 4bd5c6b to 214da08 Compare June 15, 2021 14:55

DoNotPanicUA linked an issue Jun 17, 2021 that may be closed by this pull request

🎉 New level of abstraction for relational database sources #4024

Closed

DoNotPanicUA changed the title ~~Aleonets/4024 abstract source~~ 🎉 Abstract level for SQL relational database sources Jun 17, 2021

yaroslav-hrytsaienko approved these changes Jun 22, 2021

View reviewed changes

DoNotPanicUA added 12 commits June 22, 2021 17:18

unfinished jdbcsource separation

286e327

creation AbstactRelation

6a62eaa

Migrate StateManager to new abstract level (JdbcSource -> RelationalS…

0aaf904

…ource)

fix imports

6dcedf5

move configs to Database level + fix MySql source

6d8e976

make in line jdbc source with a new impl

042c527

Fix ScaffoldJavaJdbcSource template

147e166

rename AbstractField to CommonField. Now it

246528a

s not an abstract class. + add default implementation for `AbstractRelationalDbSource.getFullyQualifiedTableName`

format

e8d29c9

rename generated files in line with their location

111abae

bonus renaming

3bc83b2

move utility methods specific for jdbc source to a proper module

18e86d8

DoNotPanicUA force-pushed the aleonets/4024-abstract-source branch from 98c4f5e to 18e86d8 Compare June 22, 2021 19:17

internal review update

67f1e6a

DoNotPanicUA marked this pull request as ready for review June 22, 2021 21:00

DoNotPanicUA requested review from subodh1810 and sherifnada June 22, 2021 21:01

remove data type parameter limitation + rename class parameters

c7d2eff

DoNotPanicUA added 3 commits June 23, 2021 22:37

Merge remote-tracking branch 'origin/master' into aleonets/4024-abstr…

d16f883

…act-source # Conflicts: # airbyte-integrations/connectors/source-mysql/src/main/java/io/airbyte/integrations/source/mysql/MySqlSource.java

make DB2 in line with new relational abstract classes

75e7c99

add missing import

3dc5383

subodh1810 reviewed Jun 25, 2021

View reviewed changes

DoNotPanicUA requested a review from subodh1810 June 29, 2021 21:57

Merge remote-tracking branch 'origin/master' into 4024-abstract-source

b3e0801

close unused connections

5421313

DoNotPanicUA added 4 commits July 2, 2021 17:19

Merge remote-tracking branch 'origin/master' into aleonets/4024-abstr…

938b5ed

…act-source

Make cockroachdb in line with the PR

8029fe9

format

4e7d1a6

format

af88ce6

subodh1810 approved these changes Jul 5, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into aleonets/4024-abstr…

c038177

…act-source

DoNotPanicUA mentioned this pull request Jul 5, 2021

Refactor source database creation #4547

Closed

DoNotPanicUA merged commit 107f5b8 into master Jul 5, 2021

DoNotPanicUA deleted the aleonets/4024-abstract-source branch July 5, 2021 14:18

karinakuz added connectors/sources-database connectors/source/db2 labels Jan 4, 2022

karinakuz added connectors/destinations-warehouse connectors/destination/snowflake labels Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 Abstract level for SQL relational database sources #4123

🎉 Abstract level for SQL relational database sources #4123

DoNotPanicUA commented Jun 15, 2021 •

edited

yaroslav-hrytsaienko left a comment

yaroslav-hrytsaienko Jun 22, 2021

yaroslav-hrytsaienko Jun 22, 2021

yaroslav-hrytsaienko Jun 22, 2021

subodh1810 commented Jun 23, 2021

subodh1810 left a comment

subodh1810 Jun 25, 2021

DoNotPanicUA Jun 29, 2021

subodh1810 Jun 25, 2021

DoNotPanicUA Jun 29, 2021

subodh1810 Jun 25, 2021

DoNotPanicUA Jun 29, 2021

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 2, 2021 •

edited by github-actions bot

subodh1810 left a comment

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021

DoNotPanicUA commented Jul 5, 2021


		public abstract class SqlDatabase implements AutoCloseable {

		private JsonNode sourceConfig;

🎉 Abstract level for SQL relational database sources #4123

🎉 Abstract level for SQL relational database sources #4123

Conversation

DoNotPanicUA commented Jun 15, 2021 • edited

What

How

Recommended reading order

Pre-merge Checklist

yaroslav-hrytsaienko 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

subodh1810 commented Jun 23, 2021

subodh1810 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

DoNotPanicUA commented Jun 30, 2021 • edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 • edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 2, 2021 • edited by github-actions bot

subodh1810 left a comment

Choose a reason for hiding this comment

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 • edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021

DoNotPanicUA commented Jul 5, 2021

DoNotPanicUA commented Jun 15, 2021 •

edited

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jun 30, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 2, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot

DoNotPanicUA commented Jul 5, 2021 •

edited by github-actions bot