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.

Sign up for GitHub

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

Jump to bottom

[Core] Log restore errors by default #25172

Closed
wants to merge 2 commits into from
Closed

[Core] Log restore errors by default #25172

wants to merge 2 commits into from

Conversation

kennethlien
Copy link
Contributor

Why are these changes needed?

Failed restores result in Ray silently hanging. This should alert users when restores have failed

Related issue number

#24248

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@stephanie-wang stephanie-wang self-assigned this May 25, 2022
stephanie-wang
stephanie-wang approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@stephanie-wang stephanie-wang left a comment

Choose a reason for hiding this comment

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

Huh, not sure why this was DEBUG only before... :D

@stephanie-wang
Copy link
Contributor

cc @rkooo567 in case he knows why.

@kennethlien kennethlien changed the title Log restore errors by default [Core] Log restore errors by default May 25, 2022
@stephanie-wang
Copy link
Contributor

@kennethlien could you address the lint errors? You can use ./ci/lint/format.sh.

@kennethlien
Copy link
Contributor Author

@stephanie-wang Whoops, fixed!

@rkooo567
Copy link
Contributor

I think this was extremely noisy when the failure actually happens because we do infinite retry. I think we should go this direction instead of just turning the log #25000.

@rkooo567
Copy link
Contributor

So I prefer not to merge this, but if you feel strong about this, please go ahead. But I'd prefer to make progress on this PR asap #25000 instead of merging this.

@kennethlien
Copy link
Contributor Author

We're awaiting a response on #24248 (comment), not too sure how to proceed with related work for #25000.

@stephanie-wang stephanie-wang added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! labels Jun 6, 2022
@stale
Copy link

stale bot commented Jul 6, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 6, 2022
@rkooo567
Copy link
Contributor

rkooo567 commented Jul 25, 2022 •
edited
Loading

We will take over this task if there's no progress. cc @rickyyx (I am thinking this is important as a part of state improvement after 2.0 branch cut). Let's discuss in person

@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Jul 25, 2022
@rickyyx rickyyx self-assigned this Jul 25, 2022
@stale
Copy link

stale bot commented Sep 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 8, 2022
@rickyyx rickyyx removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 16, 2022
@stale
Copy link

stale bot commented Oct 18, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 18, 2022
@stale
Copy link

stale bot commented Nov 9, 2022

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

@stale stale bot closed this Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanie-wang stephanie-wang stephanie-wang approved these changes

Assignees

@stephanie-wang stephanie-wang

@rickyyx rickyyx

@rkooo567 rkooo567

Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! stale The issue is stale. It will be closed within 7 days unless there are further conversation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kennethlien @stephanie-wang @rkooo567 @rickyyx