-
Notifications
You must be signed in to change notification settings - Fork 0
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
Retry on error_apple non-terminal error #1148
base: main
Are you sure you want to change the base?
Conversation
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.
A couple of questions, but looks right
@@ -19,7 +19,7 @@ class PublishingPipelineState < ApplicationRecord | |||
scope :latest_failed_pipelines, -> { | |||
# Grab the latest attempted Publishing Item AND the latest failed Pub Item. | |||
# If that is a non-null intersection, then we have a current/latest+failed pipeline. | |||
where(publishing_queue_item_id: PublishingQueueItem.latest_attempted.latest_failed.select(:id)) | |||
where(publishing_queue_item_id: PublishingQueueItem.latest_attempted.latest_failed.order(id: :asc).select(:id)) |
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.
is that order(:id) appropriate to add to either the latest_attempted
or latest_failed
scopes?
Also, why :asc? That means the latest will be at the end? Wouldn't the most recently failed make sense as the first?
def self.retry_failed_pipelines! | ||
Podcast.where(id: latest_failed_pipelines.select(:podcast_id).distinct).each do |podcast| | ||
latest_failed_podcasts.each do |podcast| |
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.
yeah, nicer to have a method for this.
@@ -1,6 +1,6 @@ | |||
class PublishingPipelineState < ApplicationRecord | |||
TERMINAL_STATUSES = [:complete, :error, :expired].freeze | |||
TERMINAL_FAILURE_STATUSES = [:error, :expired].freeze | |||
FAILURE_STATUSES = [:error, :expired, :error_apple].freeze |
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.
nice, makes sense to have this grouping
Closes #1134
This includes the intermediate
error_apple
in the list of retryable error states. Prior to this, we had only considered terminal error states. With this PR, we will retry publishing pipelines having intermediateerror_apple
states and terminalcomplete
(success) states.