🎉Destination-s3: added support for AWS Glue crawler #11173

etsybaev · 2022-03-15T18:26:01Z

What

A user wants to use the AWS Glue crawler feature, but currently, all special symbols transform to "_" symbol.

How

Updated logic to do not transform special symbols in stream name if user adds the "=" symbol in prefix.

Tested locally:

🚨 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 by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory 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

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.

etsybaev · 2022-03-15T19:01:04Z

/test connector=connectors/destination-s3

…-for-aws-glue-crawler

alexandr-shegeda

LGTM. please reformat the code and update the related documentation

etsybaev · 2022-03-17T15:04:45Z

/test connector=connectors/destination-s3

etsybaev · 2022-03-17T18:42:36Z

/test connector=connectors/destination-s3

ChristopheDuong · 2022-03-21T14:12:30Z

...ination-s3/src/main/java/io/airbyte/integrations/destination/s3/util/S3OutputPathHelper.java

+      // more details https://docs.aws.amazon.com/glue/latest/dg/crawler-s3-folder-table-partition.html
+      paths.add(streamName);
+    }else {
+      paths.add(NAME_TRANSFORMER.convertStreamName(streamName));


Shouldn't we create a specific NamingConventionTransformer nameTransformer for S3 when instantiating the S3Destination?
It would be S3NameTransformer instead of a generic ExtendedNameTransformer:

airbyte/airbyte-integrations/connectors/destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3DestinationConstants.java

Line 12 in ea92a24

public static final ExtendedNameTransformer NAME_TRANSFORMER = new ExtendedNameTransformer();

That would allow us to specify what are the allowed characters in the bucket name/path such as = or other characters, no?
Non-allowed characters are then following the rule:

all special symbols transform to "_" symbol.

Here are some examples of how it's done in other connectors:

Postgres:

airbyte/airbyte-integrations/connectors/destination-postgres/src/main/java/io/airbyte/integrations/destination/postgres/PostgresDestination.java

Line 38 in ea92a24

super(DRIVER_CLASS, new PostgresSQLNameTransformer(), new PostgresSqlOperations());

Snowflake:

airbyte/airbyte-integrations/connectors/destination-snowflake/src/main/java/io/airbyte/integrations/destination/snowflake/SnowflakeInternalStagingDestination.java

Line 33 in ea92a24

this(new SnowflakeSQLNameTransformer());

MongoDB:

airbyte/airbyte-integrations/connectors/destination-mongodb/src/main/java/io/airbyte/integrations/destination/mongodb/MongodbDestination.java

Line 64 in ea92a24

namingResolver = new MongodbNameTransformer();

etc

Or at least, when we construct the consumer, we pass it to the factory:

Redshift:

airbyte/airbyte-integrations/connectors/destination-redshift/src/main/java/io/airbyte/integrations/destination/redshift/RedshiftCopyS3Destination.java

Line 59 in ea92a24

return new RedshiftSQLNameTransformer();

WDYT @tuliren?

I agree. We should hide that complexity in a custom NameTransformer

As far as I understood, this converter is used for creating a stream name only. A user wants to use some specific feature([AWS Glue crawler). That's why I removed any auto-replacement if "=" symbol is used

@etsybaev the logic is sound, but we should encapsulate it into a S3NameTransformer

girarda · 2022-03-21T18:11:38Z

...ination-s3/src/main/java/io/airbyte/integrations/destination/s3/util/S3OutputPathHelper.java

+      // more details https://docs.aws.amazon.com/glue/latest/dg/crawler-s3-folder-table-partition.html
+      paths.add(streamName);
+    }else {
+      paths.add(NAME_TRANSFORMER.convertStreamName(streamName));


I agree. We should hide that complexity in a custom NameTransformer

…-for-aws-glue-crawler

etsybaev · 2022-03-23T14:24:47Z

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2028994172
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2028994172
No Python unittests run

...tination-s3/src/main/java/io/airbyte/integrations/destination/s3/util/S3NameTransformer.java

etsybaev · 2022-03-23T17:15:59Z

/publish connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2029751587
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2029751587

[4654] Destination-s3: added support for AWS Glue crawler

226715e

github-actions bot added the area/connectors Connector related issues label Mar 15, 2022

etsybaev mentioned this pull request Mar 15, 2022

Destination S3: Allow special chars for table_prefix #4654

Closed

etsybaev requested a review from alexandr-shegeda March 15, 2022 18:49

etsybaev changed the title ~~Destination-s3: added support for AWS Glue crawler~~ 🎉Destination-s3: added support for AWS Glue crawler Mar 15, 2022

Merge branch 'master' into etsybaev/4654-destination-s3-added-support…

92a1857

…-for-aws-glue-crawler

etsybaev temporarily deployed to more-secrets March 16, 2022 11:46 Inactive

etsybaev requested review from VitaliiMaltsev and removed request for alexandr-shegeda March 16, 2022 12:39

VitaliiMaltsev approved these changes Mar 16, 2022

View reviewed changes

alexandr-shegeda self-requested a review March 16, 2022 12:52

alexandr-shegeda approved these changes Mar 16, 2022

View reviewed changes

etsybaev linked an issue Mar 17, 2022 that may be closed by this pull request

Destination S3: Allow special chars for table_prefix #4654

Closed

etsybaev temporarily deployed to more-secrets March 17, 2022 13:17 Inactive

etsybaev requested a review from girarda March 17, 2022 15:56

etsybaev marked this pull request as ready for review March 18, 2022 13:18

ChristopheDuong reviewed Mar 21, 2022

View reviewed changes

girarda requested changes Mar 21, 2022

View reviewed changes

etsybaev added 2 commits March 23, 2022 15:28

Merge branch 'master' into etsybaev/4654-destination-s3-added-support…

7fc0767

…-for-aws-glue-crawler

Fixed code sd per comments in PR

fc820ee

girarda reviewed Mar 23, 2022

View reviewed changes

...tination-s3/src/main/java/io/airbyte/integrations/destination/s3/util/S3NameTransformer.java Show resolved Hide resolved

girarda approved these changes Mar 23, 2022

View reviewed changes

Updated doc and bumped version

e4d4d26

etsybaev temporarily deployed to more-secrets March 23, 2022 17:14 Inactive

Updated doc

fea45e1

github-actions bot added the area/documentation Improvements or additions to documentation label Mar 23, 2022

etsybaev merged commit bf9a189 into master Mar 23, 2022

etsybaev deleted the etsybaev/4654-destination-s3-added-support-for-aws-glue-crawler branch March 23, 2022 18:31

ChristopheDuong mentioned this pull request Mar 23, 2022

🎉 Change destination-s3 buffering to reduce/stabilize memory/thread consumption #11294

Merged

40 tasks

This was referenced Mar 23, 2022

Bump Airbyte version from 0.35.59-alpha to 0.35.60-alpha #11373

Closed

Bump Airbyte version from 0.35.59-alpha to 0.35.60-alpha #11402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉Destination-s3: added support for AWS Glue crawler #11173

🎉Destination-s3: added support for AWS Glue crawler #11173

etsybaev commented Mar 15, 2022 •

edited

Loading

etsybaev commented Mar 15, 2022

alexandr-shegeda left a comment

etsybaev commented Mar 17, 2022

etsybaev commented Mar 17, 2022

ChristopheDuong Mar 21, 2022 •

edited

Loading

girarda Mar 21, 2022

etsybaev Mar 21, 2022

girarda Mar 21, 2022

girarda Mar 21, 2022

etsybaev commented Mar 23, 2022 •

edited by github-actions bot

Loading

etsybaev commented Mar 23, 2022 •

edited by github-actions bot

Loading

🎉Destination-s3: added support for AWS Glue crawler #11173

🎉Destination-s3: added support for AWS Glue crawler #11173

Conversation

etsybaev commented Mar 15, 2022 • edited Loading

What

How

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

etsybaev commented Mar 15, 2022

alexandr-shegeda left a comment

Choose a reason for hiding this comment

etsybaev commented Mar 17, 2022

etsybaev commented Mar 17, 2022

ChristopheDuong Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

girarda Mar 21, 2022

Choose a reason for hiding this comment

etsybaev Mar 21, 2022

Choose a reason for hiding this comment

girarda Mar 21, 2022

Choose a reason for hiding this comment

girarda Mar 21, 2022

Choose a reason for hiding this comment

etsybaev commented Mar 23, 2022 • edited by github-actions bot Loading

etsybaev commented Mar 23, 2022 • edited by github-actions bot Loading

etsybaev commented Mar 15, 2022 •

edited

Loading

ChristopheDuong Mar 21, 2022 •

edited

Loading

etsybaev commented Mar 23, 2022 •

edited by github-actions bot

Loading

etsybaev commented Mar 23, 2022 •

edited by github-actions bot

Loading