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

Quiet job continuation #1133

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Quiet job continuation #1133

wants to merge 16 commits into from

Conversation

svevang
Copy link
Member

@svevang svevang commented Oct 18, 2024

After #1116 and #1122

Quietly retry apple jobs and send alerts (error+fatal log lines) in the case of long waits

@svevang svevang changed the base branch from main to limit-error-interface October 18, 2024 16:37
@svevang svevang marked this pull request as ready for review October 28, 2024 19:18
Base automatically changed from limit-error-interface to main November 4, 2024 16:00
Copy link
Member

@kookster kookster left a comment

Choose a reason for hiding this comment

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

had a few questions!

app/jobs/publish_feed_job.rb Show resolved Hide resolved
app/jobs/publish_feed_job.rb Show resolved Hide resolved
when "rss" then [:error_rss!, :warn, true]
when "apple_timeout"
level = apple_timeout_log_level(error)
[:retry!, level, level == :fatal || level == :error]
Copy link
Member

Choose a reason for hiding this comment

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

I might refactor to %i[error fatal].include?(level), but this makes sense as the place to decide to raise the error or not.
Another question that's not a request for change - does it feel like this is info I wish was on the error somehow, like how the error has a log level, can it also have a raise or not? maybe that is not a good responsibility for the error, idk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re: clarity, Good point!

Re: error interface responsibility. Hrm, yes, I see what you're getting at. In essence, we're already responding to the error as the controlling parameter here, so why not push that should_raise? responsibility up to the error class itself...

app/models/apple/episode_delivery_status.rb Outdated Show resolved Hide resolved
Copy link
Member

@kookster kookster left a comment

Choose a reason for hiding this comment

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

yeah, those changes to raise logic make sense to me (and removing some code too!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants