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

feat: cron service #3399

Merged
merged 1 commit into from
Nov 15, 2024
Merged

feat: cron service #3399

merged 1 commit into from
Nov 15, 2024

Conversation

alecthomas
Copy link
Collaborator

Functional but not not completely wired up. Tests pass, metrics aren't hooked up, no main service binary.

@alecthomas alecthomas requested a review from matt2e November 15, 2024 03:15
This was referenced Nov 15, 2024
Functional but not not completely wired up. Tests pass, metrics aren't hooked
up, no main service binary.
orderQueue(cronQueue)

if err := callCronJob(ctx, verbClient, job); err != nil {
logger.Errorf(err, "Failed to execute cron job")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we're dropping support for retry policies on cron? We should probably make that a validation check to ensure no retry directives on cron job verbs if so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe just a ticket to add retry logic back in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems likely we'll be moving retries to Istio.

@matt2e
Copy link
Collaborator

matt2e commented Nov 15, 2024

Looks sweet and relatively simple 👍

@alecthomas alecthomas merged commit a42ae6d into main Nov 15, 2024
92 checks passed
@alecthomas alecthomas deleted the aat/cron-service branch November 15, 2024 04:18
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