-
Notifications
You must be signed in to change notification settings - Fork 443
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
Merged log files in Step Ops steps might be not available on main process, due to merge in the step op #2795
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
This changes the logic of the merging right: Previously we only merged the last X log files, which means we would not end up reading/writing the full logs every time we merge. Unless I'm mistaken, this is now different and we always
- load all previous log files
- write them to a single merged file
For long-running steps, won't this again be an issue that it reads and writes potentially many megabytes of logs every few seconds?
Valid concerns, thanks! I reworked this to be less heavy for the execution:
|
Describe changes
I fixed the max recursion issue with step-op controlled steps on the main process.
Additionally, I simplified the merge function to avoid such issues in general.
There is one BIG ASSUMPTION here: time on client and step op should be quite in sync, otherwise the order of the log might go crazy. Generally, this is not a super critical thing, since in the worst case it can plug all client logs to the end or to the beginning of the log file, so the major logs from step op will flow in chronological order.
@bcdurak this one also should solve the retry missing logs thingy.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes