JDBC Destinations: improve error message for conflicting streams #21342

edgao · 2023-01-12T18:47:11Z

What

see https://github.com/airbytehq/airbyte-internal-issues/issues/2542 for information

I'm only planning to publish destination-snowflake; other destinations can get published whenever 🤷

How

In jdbc destination connectors, detect multiple tables writing into the same table (currently this just throws IllegalStateException) and throw a more informative config error.

🚨 User Impact 🚨

none, this is just improving how we handle an already broken situation

Pre-merge Checklist

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

github-actions · 2023-01-12T18:49:03Z

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

Run integration tests
Bump connector or module version
Add changelog
Publish the new version

✅ Sources (0)

Connector	Version	Changelog	Publish

See "Actionable Items" below for how to resolve warnings and errors.

❌ Destinations (21)

Connector	Version	Changelog	Publish
`destination-azure-blob-storage`	`0.1.6`	✅	✅
`destination-clickhouse`	`0.2.1`	✅	✅
`destination-clickhouse-strict-encrypt`	`0.2.1`	🔵 (ignored)	🔵 (ignored)
`destination-databricks`	`0.3.1`	✅	✅
`destination-dynamodb`	`0.1.7`	✅	✅
`destination-gcs`	`0.2.12`	✅	✅
`destination-mariadb-columnstore`	`0.1.7`	✅	✅
`destination-mssql`	`0.1.22`	✅	✅
`destination-mssql-strict-encrypt`	`0.1.22`	🔵 (ignored)	🔵 (ignored)
`destination-mysql`	`0.1.20`	✅	✅
`destination-mysql-strict-encrypt`	❌ `0.1.21` (mismatch: `0.1.20`)	🔵 (ignored)	🔵 (ignored)
`destination-oracle`	`0.1.19`	✅	✅
`destination-oracle-strict-encrypt`	`0.1.19`	🔵 (ignored)	🔵 (ignored)
`destination-postgres`	`0.3.26`	✅	✅
`destination-postgres-strict-encrypt`	`0.3.26`	🔵 (ignored)	🔵 (ignored)
`destination-redshift`	`0.3.53`	✅	✅
`destination-rockset`	`0.1.4`	✅	✅
`destination-snowflake`	`0.4.42`	❌ (changelog missing)	✅
`destination-teradata`	`0.1.0`	✅	✅
`destination-tidb`	`0.1.0`	✅	✅
`destination-yugabytedb`	`0.1.0`	✅	✅

See "Actionable Items" below for how to resolve warnings and errors.

👀 Other Modules (1)

base-normalization

Actionable Items

(click to expand)

Category	Status	Actionable Item
Version	❌ mismatch	The version of the connector is different from its normal variant. Please bump the version of the connector.
	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
Changelog	⚠ doc not found	The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like `source-jdbc` is not published or documented). Please double-check to make sure that it is not a bug.
	❌ changelog missing	There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish	⚠ not in seed	The connector is not in the seed file (e.g. `source_definitions.yaml`), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.
	❌ diff seed version	The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the `/publish` command to publish the latest version.

edgao · 2023-01-12T18:50:31Z

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/3905155704
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/3905155704
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 14      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    14      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     166      8    95%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         189     48    75%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         61     38    38%
normalization/transform_catalog/stream_processor.py                 595    400    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1441    629    56%

Build Passed

Test summary info:

All Passed

ryankfu · 2023-01-12T19:10:36Z

...n-jdbc/src/main/java/io/airbyte/integrations/destination/staging/StagingConsumerFactory.java

-        writeConfigs.stream()
-            .collect(Collectors.toUnmodifiableMap(
-                StagingConsumerFactory::toNameNamespacePair, Function.identity()));
+    final Set<WriteConfig> conflictingStreams = new HashSet<>();


This block seems to make sense in the onStartFunction since none of the values you're accessing here seem to be only isolated to the flushBufferFunction section

The reason that this makes sense to me in the onStartFunction is primarily because you already have the writeConfig values (since we're setting up the temporary tables for each stream (synonymous with writeConfig) and the metadata for each streamIdentifier will be known at setup time

It also fits with what onStartFunction means which is set up so this is only called once as opposed to many times throughout the flushing of the buffer (N times where N can be infinitely large)

this block happens before the lambda gets created (i.e. it's not actually part of the flush buffer function), so I think it only gets executed once per sync

That does make sense, although I would be hesitant to have this in the flushBufferFunction since it muddies the singular responsibility of the buffer flush. Also, since we know that the lambda for onStartFunction is called only once and it already includes writeConfigs that it would be another suitable location

That said, this isn't blocking

ryankfu · 2023-01-12T19:35:36Z

...n-jdbc/src/main/java/io/airbyte/integrations/destination/staging/StagingConsumerFactory.java

-        writeConfigs.stream()
-            .collect(Collectors.toUnmodifiableMap(
-                StagingConsumerFactory::toNameNamespacePair, Function.identity()));
+    final Set<WriteConfig> conflictingStreams = new HashSet<>();


That does make sense, although I would be hesitant to have this in the flushBufferFunction since it muddies the singular responsibility of the buffer flush. Also, since we know that the lambda for onStartFunction is called only once and it already includes writeConfigs that it would be another suitable location

That said, this isn't blocking

edgao · 2023-01-12T20:43:33Z

submitted https://github.com/airbytehq/airbyte-internal-issues/issues/2543 to solve for non-jdbc destinations

edgao · 2023-01-12T22:18:52Z

/publish connector=connectors/destination-snowflake

🕑 Publishing the following connectors:
connectors/destination-snowflake
https://github.com/airbytehq/airbyte/actions/runs/3906501628

Connector	Did it publish?	Were definitions generated?
connectors/destination-snowflake	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

) * catch conflicting streams as configerror * add test * bump version + changelog * derp, fix test setup * derp * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

edgao added 2 commits January 12, 2023 10:33

catch conflicting streams as configerror

5e31b4e

add test

182d416

octavia-squidington-iv added area/connectors Connector related issues connectors/destination/jdbc labels Jan 12, 2023

edgao temporarily deployed to more-secrets January 12, 2023 18:49 — with GitHub Actions Inactive

edgao requested a review from ryankfu January 12, 2023 18:51

edgao marked this pull request as ready for review January 12, 2023 18:51

edgao requested a review from a team as a code owner January 12, 2023 18:51

ryankfu reviewed Jan 12, 2023

View reviewed changes

ryankfu approved these changes Jan 12, 2023

View reviewed changes

edgao added 2 commits January 12, 2023 12:44

bump version + changelog

8707685

Merge branch 'master' into edgao/config_error_duplicate_stream

6deb8b5

octavia-squidington-iv added area/documentation Improvements or additions to documentation connectors/destination/snowflake labels Jan 12, 2023

edgao temporarily deployed to more-secrets January 12, 2023 20:47 — with GitHub Actions Inactive

edgao added 2 commits January 12, 2023 13:23

derp, fix test setup

a678556

derp

281b094

edgao temporarily deployed to more-secrets January 12, 2023 21:25 — with GitHub Actions Inactive

auto-bump connector version

026775d

edgao enabled auto-merge (squash) January 12, 2023 23:43

octavia-squidington-iii temporarily deployed to more-secrets January 12, 2023 23:44 — with GitHub Actions Inactive

edgao merged commit 1e44c34 into master Jan 13, 2023

edgao deleted the edgao/config_error_duplicate_stream branch January 13, 2023 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC Destinations: improve error message for conflicting streams #21342

JDBC Destinations: improve error message for conflicting streams #21342

edgao commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

✅ Sources (0)

❌ Destinations (21)

👀 Other Modules (1)

Actionable Items

edgao commented Jan 12, 2023 •

edited by github-actions bot

Loading

ryankfu Jan 12, 2023

edgao Jan 12, 2023

ryankfu Jan 12, 2023

ryankfu Jan 12, 2023

edgao commented Jan 12, 2023

edgao commented Jan 12, 2023 •

edited by github-actions bot

Loading

JDBC Destinations: improve error message for conflicting streams #21342

JDBC Destinations: improve error message for conflicting streams #21342

Conversation

edgao commented Jan 12, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

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

Affected Connector Report

✅ Sources (0)

❌ Destinations (21)

👀 Other Modules (1)

Actionable Items

edgao commented Jan 12, 2023 • edited by github-actions bot Loading

Build Passed

ryankfu Jan 12, 2023

Choose a reason for hiding this comment

edgao Jan 12, 2023

Choose a reason for hiding this comment

ryankfu Jan 12, 2023

Choose a reason for hiding this comment

ryankfu Jan 12, 2023

Choose a reason for hiding this comment

edgao commented Jan 12, 2023

edgao commented Jan 12, 2023 • edited by github-actions bot Loading

edgao commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

edgao commented Jan 12, 2023 •

edited by github-actions bot

Loading

edgao commented Jan 12, 2023 •

edited by github-actions bot

Loading