Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convert destination-snowflake to Kotlin CDK #36910

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented Apr 8, 2024

not only bringing snowflake to the latest CDK but also:

  1. Bringing the SourceOperation into production code from the test code. There's really no reason those improvements should stay out of production (and they're present in the source-snowflake)
  2. adding putTimestamp into the SourceOperation, so that snowflake doesn't throw an exception at every call, which implies it also creates a new thread
  3. make use of the newly added ability to filter orphan thread on shutdown. We filter all the threads created during calls to SFStatement.close()
  4. don't always take a lock when deleting destinationStates. We now check if there's any states to delete by doing a SELECT (and not taking any table lock) before issuing the DELETE (the old behavior was causing test contention, and it's a bad idea in general)
  5. only execute airbyte_internal._airbyte_destination_state

Copy link

vercel bot commented Apr 8, 2024

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 May 3, 2024 4:35pm

@stephane-airbyte stephane-airbyte marked this pull request as ready for review April 8, 2024 21:59
@stephane-airbyte stephane-airbyte requested review from a team as code owners April 8, 2024 21:59
@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit labels Apr 8, 2024
Copy link
Contributor Author

stephane-airbyte commented Apr 8, 2024

@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from 625e34b to d5df838 Compare April 8, 2024 22:01
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch 9 times, most recently from 24497d2 to 00c0d0c Compare April 10, 2024 19:08
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are green 🎉

@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch 7 times, most recently from a44cb0d to 476ad08 Compare April 29, 2024 22:51
@octavia-squidington-iv octavia-squidington-iv requested a review from a team May 2, 2024 19:01
build.gradle Outdated
@@ -143,6 +143,10 @@ allprojects {
// see IntegrationRunner.stopOrphanedThreads
jvmArgs "--add-opens=java.base/java.lang=ALL-UNNAMED"

// This is required for our dangling thread checks at the end of a connector run.
// see IntegrationRunner.stopOrphanedThreads
jvmArgs "--add-opens=java.base/java.lang=ALL-UNNAMED"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems redundant considering the changes in the previous PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's needed for tests. We have some tests that use the IntegrationRunner without docker. I moved it to the PR below though

@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 997351f to 43a242e Compare May 2, 2024 19:25
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from 61cf9eb to a3f9263 Compare May 2, 2024 19:25
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 43a242e to f3f7b10 Compare May 2, 2024 19:27
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from a3f9263 to 000cb1f Compare May 2, 2024 19:27
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from f3f7b10 to 06bcd3a Compare May 2, 2024 19:40
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from 000cb1f to 5c32fa9 Compare May 2, 2024 19:41
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 06bcd3a to 961d513 Compare May 2, 2024 20:09
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch 2 times, most recently from d1ed374 to 97d979d Compare May 2, 2024 20:12
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 961d513 to 55bf52e Compare May 2, 2024 20:47
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch 2 times, most recently from dd9a14a to 0784c79 Compare May 2, 2024 20:54
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 55bf52e to 41beb7a Compare May 2, 2024 21:09
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch 2 times, most recently from 20c12cd to b344443 Compare May 3, 2024 05:35
Copy link
Contributor Author

stephane-airbyte commented May 3, 2024

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/8934823413
✅ Successfully published Java CDK version=0.31.7!

Base automatically changed from stephane/05-01-debuggability_improvements_to_the_cdk to master May 3, 2024 13:50
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from b344443 to 0a3d4d7 Compare May 3, 2024 13:52
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from b2790df to 0a3d4d7 Compare May 3, 2024 16:34
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch from 0a3d4d7 to 80bf9c6 Compare May 3, 2024 16:35
@stephane-airbyte stephane-airbyte merged commit 7c0a6c5 into master May 3, 2024
32 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/04-08-convert_destination-snowflake_to_kotlin_cdk branch May 3, 2024 18:20
aasimsani pushed a commit to taxwire/airbyte that referenced this pull request May 6, 2024
not only bringing snowflake to the latest CDK but also:
1) Bringing the `SourceOperation` into production code from the test code. There's really no reason those improvements should stay out of production (and they're present in the source-snowflake)
2) adding `putTimestamp` into the `SourceOperation`, so that snowflake doesn't throw an exception at every call, which implies it also creates a new thread
3) make use of the newly added ability to filter orphan thread on shutdown. We filter all the threads created during calls to `SFStatement.close()`
4) don't always take a lock when deleting destinationStates. We now check if there's any states to delete by doing a `SELECT` (and not taking any table lock) before issuing the `DELETE` (the old behavior was causing test contention, and it's a bad idea in general)
5) only execute `airbyte_internal._airbyte_destination_state`
clnoll pushed a commit that referenced this pull request May 10, 2024
not only bringing snowflake to the latest CDK but also:
1) Bringing the `SourceOperation` into production code from the test code. There's really no reason those improvements should stay out of production (and they're present in the source-snowflake)
2) adding `putTimestamp` into the `SourceOperation`, so that snowflake doesn't throw an exception at every call, which implies it also creates a new thread
3) make use of the newly added ability to filter orphan thread on shutdown. We filter all the threads created during calls to `SFStatement.close()`
4) don't always take a lock when deleting destinationStates. We now check if there's any states to delete by doing a `SELECT` (and not taking any table lock) before issuing the `DELETE` (the old behavior was causing test contention, and it's a bad idea in general)
5) only execute `airbyte_internal._airbyte_destination_state`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/destination/snowflake connectors/source/dynamodb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants