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

Retry on error_apple non-terminal error #1148

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

Conversation

svevang
Copy link
Member

@svevang svevang commented Oct 30, 2024

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 intermediate error_apple states and terminal complete (success) states.

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.

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))
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. The intent here was to order the output of the scope. But this attempt doesn't map cleanly between the two models (Queue and PublishingState). Removed!

Early on, I opted to avoid a base scope that defines ordering (IIRC my attempt caused some SQL slowness). Lately, I've been wishing that the output of these scopes follows a natural ordering (like :created, :rss, :completed) instead of a random ordering (as can happen with joins and aggregation).

In the console and in tests, its much easier to read the state transitions in the order they occurred.

app/models/publishing_pipeline_state.rb Show resolved Hide resolved
app/models/publishing_pipeline_state.rb 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.

looks good! I probably will have a little fun merging this with the megaphone changes, but I think it should be fine - maybe error_integration ends up one of those non-terminal failure states... I'll figure it out, this looks good!

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.

Retry delegated delivery publishing errors when sync_blocks_rss == false
2 participants