fix: fix PrintBuffer logic #336
Conversation
Signed-off-by: Artem Inzhyyants <[email protected]>
|
TODO:
|
|
!!!!1111 thank you Artem! <3 |
|
UPD: pinned and tested on stripe test WS — no errors |
📝 WalkthroughWalkthroughThe changes update how output is buffered in the Airbyte CDK. In Changes
Sequence Diagram(s)sequenceDiagram
participant L as launch()
participant PB as PRINT_BUFFER
participant AE as AirbyteEntrypoint.run()
L->>PB: Enter context using PRINT_BUFFER
PB->>AE: Provide buffered output stream
AE->>PB: Write output (without flush=True)
PB->>PB: Automatically flush output at 0.1s intervals
Would you like any further details or additional diagrams to clarify the changes even more? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -27,7 +30,7 @@ | |||
| "handlers": { | |||
| "console": { | |||
| "class": "logging.StreamHandler", | |||
| "stream": "ext://sys.stdout", | |||
There was a problem hiding this comment.
Should we be worried that some logs would be emitted before (maybe during run.py) or after (when there is a breaking exception and we catch the uncaught exception) the with PRINT_BUFFER statement?
There was a problem hiding this comment.
I would also like a more exhaustive description to understand what caused the issue before and how this change fixes the issue.
@maxi297 , my guess is that issue was caused due to the logger.
Should we be worried that some logs would be emitted before (maybe during run.py) or after (when there is a breaking exception and we catch the uncaught exception) the with PRINT_BUFFER statement?
not really, since we share exactly the same object (stream) in both logger.write and print statements.
Signed-off-by: Artem Inzhyyants <[email protected]>
There was a problem hiding this comment.
So the problem was that PrintBuffer wasn't used as a handlers. stream in the logging config, right? As I don't have more information on the original issue, are we able to reproduce the issue that happened before, apply this change and see that we don't have the issue anymore?
I'll test it 02/24 UPD:
|
|
@maxi297 , UPD: after deeeeep Dive into logs from Brian's init sync I got the idea why it could happen and why we do not see the difference between old style implementation of PrintBuffer vs fixed one. there are many lines of Logger: Note The RLock (reentrant lock) is primarily designed to prevent data corruption when multiple threads access a shared resource (like your StringIO buffer). It ensures that only one thread can modify the buffer at a time, preventing garbled output. However, it does not provide a guarantee of delivery of every single write in the face of abrupt process termination. So my guess is that PrintBuffer tried to write to Rollout plan:
|
|
@artem1205 I would LOVE to see this in prod. Again, thank you for pushing this over the finish line. |
What
Resolve https://github.com/airbytehq/airbyte-internal-issues/issues/10136
Resolve #283
How
Summary by CodeRabbit
New Features
Bug Fixes