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

Fix stuck backfill when scheduled work queue is at capacity #5575

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Apr 12, 2024

Issue Addressed

Addresses #5504.

Fix stuck backfill when scheduled work queue is at capacity and make sure the batch is rescheduled.

Additional Info

I tried adding a regression test but can't get it to pass. Will look into it next week. (fixed)

@jimmygchen jimmygchen force-pushed the fix-stuck-backfill branch 3 times, most recently from 2964f1c to 20d0110 Compare April 12, 2024 11:43
@jimmygchen jimmygchen marked this pull request as ready for review April 12, 2024 12:16
@jimmygchen jimmygchen added the ready-for-review The code is ready for review label Apr 12, 2024
@jimmygchen jimmygchen linked an issue Apr 12, 2024 that may be closed by this pull request
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

This style of test seems like a good way to deal with timings! Nice!


// Advance the time by more than 1/2 the slot to trigger a scheduled backfill batch to be sent.
// This should fail as the `ready_work` channel is at capacity, and it should be rescheduled.
let more_than_half_slot = Duration::from_secs(slot_duration / 2 + 1);
Copy link
Member

Choose a reason for hiding this comment

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

So this is to hit the timing defined by BACKFILL_SCHEDULE_IN_SLOT, right? Can we use that directly here so as to not break this test in the future if this schedule changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in a5f76ce

@realbigsean realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 16, 2024
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 22, 2024
@realbigsean realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 22, 2024
@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented Apr 22, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 532206e

mergify bot added a commit that referenced this pull request Apr 22, 2024
@mergify mergify bot merged commit 532206e into sigp:unstable Apr 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backfill stuck on Failed to send scheduled backfill work
2 participants