-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Destinations CDK: Correctly detect when real raw/final table is correct generation during truncate sync #42503
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3175892
to
5f9fbcf
Compare
@@ -85,7 +85,7 @@ class AbstractStreamOperationTest { | |||
val initialState = | |||
mockk<DestinationInitialStatus<MinimumDestinationState.Impl>> { | |||
every { streamConfig } returns this@Truncate.streamConfig | |||
every { initialRawTableStatus } returns mockk<InitialRawTableStatus>() | |||
every { initialRawTableStatus.rawTableExists } returns false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these diffs are just to handle the new behavior. They don't affect the test in any relevant way.
dbcc2c2
to
d59f5d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
looks like line 190-198 & 201-209 are same, wondering if there is a a negative guard condition to return early (ignore if it gets ugly with !tmpTableExists || something..
)
d59f5d8
to
3d9cf10
Compare
(copying from DM)
trying to do this without that duplication yields a weird if -> if -> early return, which seems confusing. leaving it as-is |
...rc/main/kotlin/io/airbyte/integrations/base/destination/operation/AbstractStreamOperation.kt
Show resolved
Hide resolved
3d9cf10
to
341ae0d
Compare
d9889dc
to
bf62d6b
Compare
341ae0d
to
8a53e44
Compare
bf62d6b
to
aef6bf6
Compare
8a53e44
to
3922cf7
Compare
aef6bf6
to
6f61517
Compare
3922cf7
to
b9bf10d
Compare
from #42514 - this doesn't correctly handle the final table >.> i.e. the raw table is correctly preserved during the second sync, but the final table is still nuked (b/c we unconditionally create a temp final table, T+D ignores the previous sync's records b/c they have nonnull loaded_at, and then we overwrite the final table using the empty temp final table) |
cc09978
to
020d792
Compare
7fdef81
to
ad24e27
Compare
020d792
to
be63bdc
Compare
ad24e27
to
1d5f768
Compare
7d4f94e
to
4c8180c
Compare
dd2e935
to
707b94d
Compare
790075d
to
3aa5155
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
// In this case, there is no existing temp raw table, and there is a real raw table | ||
// which already belongs to the correct generation. | ||
// Check for that case now. | ||
val realStageGeneration = storageOperation.getStageGeneration(stream.id, NO_SUFFIX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait what ? 🤯
99ecb80
to
30fecb2
Compare
95ac402
to
7270022
Compare
7270022
to
840e346
Compare
/publish-java-cdk
|
from https://github.com/airbytehq/airbyte-internal-issues/issues/8784, for the new interfaces. I'll stack PRs on top of this for bigquery+redshift+snowflake.
add a new check for the real raw table during truncate sync startup, to see if it belongs to the correct generation. Also handle this new behavior during sync finalization (i.e. don't overwrite the raw table if we weren't using a temp raw table to begin with).
Adds two new unit tests for this.