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

[Dest-S3DataLake]: Bugfix: New interface setup does not always await … #56347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnny-schmidt
Copy link
Contributor

What

The load pipeline new interface waits to start a stream (StreamLoader::start) until the first time it sees data for it. However, it does not wait on setup (DestitionWriter::setup) to complete running start. This caused issues in the new BulkLoader. It really should cause issues in S3DataLake, which relies on setup, but I can't find any sign that it does. Tests all pass before and after this. However I thought I'd go ahead and break this out as a bugfix for S3DataLake specifically.

@johnny-schmidt johnny-schmidt requested a review from a team as a code owner March 23, 2025 00:19
Copy link

vercel bot commented Mar 23, 2025

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

Name Status Preview Comments Updated (UTC)
airbyte-docs 🛑 Canceled (Inspect) Mar 23, 2025 7:15am

@johnny-schmidt johnny-schmidt force-pushed the jschmidt/dest-s3-datalake/bugfix-setup-doesn-not-await-start branch from 4763f00 to 92acee8 Compare March 23, 2025 00:22
@johnny-schmidt johnny-schmidt requested a review from edgao March 24, 2025 16:00
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.

🤷 I guess we got lucky and always got the "correct" behavior out of the race condition?

(it sounds like this would have caused loud failure, rather than silent error?)

Yeah I assumed. I guess I could try injecting an artificial delay at the start of setup and see what happens. Actually I will do that right now.

@johnny-schmidt
Copy link
Contributor Author

🤷 I guess we got lucky and always got the "correct" behavior out of the race condition?
(it sounds like this would have caused loud failure, rather than silent error?)

Yeah I assumed. I guess I could try injecting an artificial delay at the start of setup and see what happens. Actually I will do that right now.

I inserted a 10 second delay and verified that setup didn't even run until after everything else was done and ... nothing broke and the tests passed 🫨

@johnny-schmidt johnny-schmidt enabled auto-merge (squash) March 24, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants