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

cdk-java: wait until after table migration before creating the WriteConfig objects #42015

Conversation

stephane-airbyte
Copy link
Contributor

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Jul 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 7:16pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Jul 16, 2024
@stephane-airbyte stephane-airbyte marked this pull request as ready for review July 16, 2024 22:25
@stephane-airbyte stephane-airbyte requested a review from a team as a July 16, 2024 22:25
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery branch from 6523058 to 469d023 Compare July 16, 2024 22:47
@stephane-airbyte stephane-airbyte requested a review from a team as a code owner July 16, 2024 22:47
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from 18f7126 to 2103175 Compare July 16, 2024 22:47
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery branch from 469d023 to ac23891 Compare July 17, 2024 14:54
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from 2103175 to f5deaa2 Compare July 17, 2024 14:54
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.

suggested a comment + slightly different structure, lgtm otherwise

)
}
val writeConfigs = mutableListOf<WriteConfig>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val writeConfigs = mutableListOf<WriteConfig>()
// This list is populated when the OnStartFunction is invoked
// which is OK, because it's only used by the flush function
// which never runs until after OnStart runs
val writeConfigs = mutableListOf<WriteConfig>()

( 😢 )

@@ -145,11 +145,14 @@ object JdbcBufferedConsumerFactory {
private fun onStartFunction(
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think of having this function return a Pair<OnStartFunction, List<WriteConfig>>?

fun onStartFunction(): Pair<OnStartFunction, List<WriteConfig>> {
  val writeConfigs = mutableList()
  val onStartFunction = OnStartFunction { ... populate writeConfigs.... }
  return Pair(onStartFunction, writeConfigs)
}

and then in createAsync, we don't need to create a mutable list manually:

val (onStartFunction, writeConfigs) = onStartFunction(...)
return AsyncStreamConsumer(
  outputRecordCollector,
  onStartFunction,
  JdbcFlushFunction(...writeConfigs...)
  ...
)

Copy link
Contributor Author

stephane-airbyte commented Jul 22, 2024

Merge activity

@stephane-airbyte stephane-airbyte force-pushed the stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery branch from ac23891 to 8891c0b Compare July 22, 2024 18:26
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from f5deaa2 to d489ef3 Compare July 22, 2024 18:26
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery branch from 8891c0b to 6893cd7 Compare July 22, 2024 18:53
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from d489ef3 to a14e7b8 Compare July 22, 2024 18:54
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery branch from 6893cd7 to c6bb2ed Compare July 22, 2024 19:08
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from a14e7b8 to f42201e Compare July 22, 2024 19:08
@stephane-airbyte stephane-airbyte changed the base branch from stephane/07-12-cdk-java_adding_a_generationid_to_sqloperations.truncatetablequery to graphite-base/42015 July 22, 2024 19:15
@graphite-app graphite-app bot changed the base branch from graphite-base/42015 to master July 22, 2024 19:15
@stephane-airbyte stephane-airbyte force-pushed the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch from f42201e to f93854a Compare July 22, 2024 19:15
@stephane-airbyte stephane-airbyte merged commit 12bf6b8 into master Jul 22, 2024
29 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/07-16-cdk-java_wait_until_after_table_migration_before_creating_the_writeconfig_objects branch July 22, 2024 19:29
xiaohansong pushed a commit that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants