Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302

etsybaev · 2022-11-10T20:20:09Z

What

Snowflake destination should set application ID in JDBC param depending on env - oss or cloud.

How

Updated connector to set it dynamically

🚨 User Impact 🚨

No breaking changes expected

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

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 by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

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

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

…nds on env - airbyte_oss or airbyte_cloud

github-actions · 2022-11-10T20:21:22Z

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 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 (1)

Connector	Version	Changelog	Publish
`destination-snowflake`	`0.4.40`	✅	✅

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

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.

etsybaev · 2022-11-10T20:21:55Z

Hi @edgao.
This PR is still in work in progress (need to run tests and check codestyle, etc), but is this what you expected from approach point of view? Thanks

edgao

nice! yeah this is exactly what I was thinking. LMK when it's ready for final review - I added a few initial comments but this looks good so far 👍

edgao · 2022-11-10T20:23:34Z

...rations/bases/base-java/src/main/java/io/airbyte/integrations/util/OssCloudEnvVarConsts.java

@@ -0,0 +1,8 @@
+package io.airbyte.integrations.util;
+
+public class OssCloudEnvVarConsts {


let's actually define these in destination-snowflake, since they're specifically what Snowflake is looking for

Moved to Destination Snowflake connector. Thanks

edgao · 2022-11-10T20:24:07Z

...wflake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestination.java

-    super(DestinationType.class, SnowflakeDestinationResolver::getTypeFromConfig, SnowflakeDestinationResolver.getTypeToDestination());
+  public SnowflakeDestination(final String airbyteEnvironment) {
+    super(DestinationType.class, SnowflakeDestinationResolver::getTypeFromConfig,
+        SnowflakeDestinationResolver.getTypeToDestination(airbyteEnvironment));
  }

  public static void main(final String[] args) throws Exception {


probably best to just delete this method entirely, to avoid confusion?

Yup, now it shouldn't be used anymore. Removed

edgao · 2022-11-10T20:25:42Z

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeInsertDestination.java

@@ -26,7 +27,7 @@ public SnowflakeInsertDestination() {

  @Override
  protected DataSource getDataSource(final JsonNode config) {
-    return SnowflakeDatabase.createDataSource(config);
+    return SnowflakeDatabase.createDataSource(config, OssCloudEnvVarConsts.AIRBYTE_OSS);


is this class even still in use? I.e. can we just delete this file completely

Nice catch. I haven't checked carefully yet, it was automatically updated by refactor IntelliIdea's feature. At first glance it's not used at all. Removed for now, will check later if it cause any issues

edgao

edgao · 2022-11-10T23:36:27Z

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationRunner.java

+    AdaptiveDestinationRunner.baseOnEnv()
+        .withOssDestination(() -> new SnowflakeDestination(OssCloudEnvVarConsts.AIRBYTE_OSS))
+        .withCloudDestination(() -> new SnowflakeDestination(OssCloudEnvVarConsts.AIRBYTE_CLOUD))
+        .run(args);


this probably also needs to do a SCHEDULED_EXECUTOR_SERVICE.shutdownNow(); similar to Akash's PR https://github.com/airbytehq/airbyte/pull/19314/files#diff-d6c8766f9065e3ddfc335cd448642c552b312f2d1feac08e62793aec3716df12R16

updated, thanks!

ryankfu · 2022-11-11T00:49:05Z

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationRunner.java

@@ -0,0 +1,16 @@
+package io.airbyte.integrations.destination.snowflake;


Missing

/* * Copyright (c) 2022 Airbyte, Inc., all rights reserved. */

Added, thanks

…ynamic-jdbc-env

etsybaev · 2022-11-11T14:13:19Z

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/3445646484
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/3445646484
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

etsybaev · 2022-11-11T15:47:49Z

/publish connector=connectors/destination-snowflake

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

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 ▶️

ryankfu · 2022-11-11T16:43:42Z

...rc/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationResolver.java

-    final SnowflakeInternalStagingDestination internalStagingDestination = new SnowflakeInternalStagingDestination();
-    final SnowflakeCopyAzureBlobStorageDestination azureBlobStorageDestination = new SnowflakeCopyAzureBlobStorageDestination();
+  public static Map<DestinationType, Destination> getTypeToDestination(
+                                                                       final String airbyteEnvironment) {


this should be on the same line as above, right?

For some reason, it started looking line that after "gradlew format" command (this command is also run in CI). I tried to make it a single line manually twice, but the automatic format sets it as you see now. Have no idea why

In that case, we can keep this change and it appears you're already in the publication process plus it's not a functional requirement. Thanks for the context though

ryankfu · 2022-11-11T16:45:58Z

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeInsertDestination.java

- * Copyright (c) 2022 Airbyte, Inc., all rights reserved.
- */
-
-package io.airbyte.integrations.destination.snowflake;


For context, through code inspect this class doesn't appear to be in use so removal makes sense

ryankfu · 2022-11-11T16:49:54Z

.../src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeDestinationRunner.java

+
+  public static void main(final String[] args) throws Exception {
+    AdaptiveDestinationRunner.baseOnEnv()
+        .withOssDestination(() -> new SnowflakeDestination(OssCloudEnvVarConsts.AIRBYTE_OSS))


not blocking, maybe could have these variables imported at the top to keep in sync with Snowflake Source

airbyte/airbyte-integrations/connectors/source-snowflake/src/main/java/io.airbyte.integrations.source.snowflake/SnowflakeSourceRunner.java

Lines 7 to 8 in 75d1353

import static io.airbyte.integrations.source.snowflake.SnowflakeDataSourceUtils.AIRBYTE_CLOUD;

import static io.airbyte.integrations.source.snowflake.SnowflakeDataSourceUtils.AIRBYTE_OSS;

In one of the comments above, I was asked to downgrade it from Base level to destination one :) I already published the connector but will have a look at my next PR. At the moment when I started working on this PR we didn't have these vars. Thanks

ryankfu

LGTM, minor comments but nothing blocking. Thanks for taking this on

…nv - airbyte_oss or airbyte_cloud (#19302) * [19250] Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud

[19250] Destination Snowflake: set jdbc application env variable depe…

eff8b5d

…nds on env - airbyte_oss or airbyte_cloud

etsybaev requested a review from a team as a code owner November 10, 2022 20:20

etsybaev removed the request for review from a team November 10, 2022 20:20

octavia-squidington-iv added the connectors/destination/snowflake label Nov 10, 2022

etsybaev requested a review from edgao November 10, 2022 20:20

edgao reviewed Nov 10, 2022

View reviewed changes

Minor refactor

4a7cda8

etsybaev requested a review from edgao November 10, 2022 20:43

edgao approved these changes Nov 10, 2022

View reviewed changes

grishick approved these changes Nov 10, 2022

View reviewed changes

edgao reviewed Nov 10, 2022

View reviewed changes

ryankfu reviewed Nov 11, 2022

View reviewed changes

Fixed checkstyle

729ba2a

octavia-squidington-iv added the area/connectors Connector related issues label Nov 11, 2022

etsybaev linked an issue Nov 11, 2022 that may be closed by this pull request

Snowflake destination should set application ID in JDBC param #19250

Closed

etsybaev changed the title ~~[WIP] Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud~~ Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud Nov 11, 2022

etsybaev added 2 commits November 11, 2022 15:12

Added missed runner

ea58ba3

Merge branch 'master' into etsybaev/19250-destination-snowflake-set-d…

826e048

…ynamic-jdbc-env

Bumped version

59c0b01

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Nov 11, 2022

ryankfu reviewed Nov 11, 2022

View reviewed changes

ryankfu approved these changes Nov 11, 2022

View reviewed changes

auto-bump connector version

5a4c88e

octavia-squidington-iii temporarily deployed to more-secrets November 11, 2022 17:05 Inactive

etsybaev merged commit 0d795d5 into master Nov 11, 2022

etsybaev deleted the etsybaev/19250-destination-snowflake-set-dynamic-jdbc-env branch November 11, 2022 17:10

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302

Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302

etsybaev commented Nov 10, 2022 •

edited

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

✅ Sources (0)

✅ Destinations (1)

Actionable Items

etsybaev commented Nov 10, 2022

edgao left a comment

edgao Nov 10, 2022

etsybaev Nov 10, 2022

edgao Nov 10, 2022

etsybaev Nov 10, 2022

edgao Nov 10, 2022

etsybaev Nov 10, 2022

edgao left a comment

edgao Nov 10, 2022

etsybaev Nov 11, 2022

ryankfu Nov 11, 2022

etsybaev Nov 11, 2022

etsybaev commented Nov 11, 2022 •

edited by github-actions bot

Loading

etsybaev commented Nov 11, 2022 •

edited by github-actions bot

Loading

ryankfu Nov 11, 2022

etsybaev Nov 11, 2022

ryankfu Nov 11, 2022

ryankfu Nov 11, 2022

ryankfu Nov 11, 2022

etsybaev Nov 11, 2022 •

edited

Loading

ryankfu left a comment

		@@ -0,0 +1,8 @@
		package io.airbyte.integrations.util;

		public class OssCloudEnvVarConsts {

		@@ -0,0 +1,16 @@
		package io.airbyte.integrations.destination.snowflake;

	import static io.airbyte.integrations.source.snowflake.SnowflakeDataSourceUtils.AIRBYTE_CLOUD;
	import static io.airbyte.integrations.source.snowflake.SnowflakeDataSourceUtils.AIRBYTE_OSS;

Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302

Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302

Conversation

etsybaev commented Nov 10, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

github-actions bot commented Nov 10, 2022 • edited Loading

Affected Connector Report

✅ Sources (0)

✅ Destinations (1)

Actionable Items

etsybaev commented Nov 10, 2022

edgao 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

edgao 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

etsybaev commented Nov 11, 2022 • edited by github-actions bot Loading

Build Passed

etsybaev commented Nov 11, 2022 • edited by github-actions bot Loading

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

etsybaev Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

ryankfu left a comment

Choose a reason for hiding this comment

etsybaev commented Nov 10, 2022 •

edited

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

etsybaev commented Nov 11, 2022 •

edited by github-actions bot

Loading

etsybaev commented Nov 11, 2022 •

edited by github-actions bot

Loading

etsybaev Nov 11, 2022 •

edited

Loading