Repository navigation
Advance pri reader ack level past expired tasks - #12473
Open
steve-weiland wants to merge 1 commit into
Open
steve-weiland wants to merge 1 commit into
steve-weiland wants to merge 1 commit into
Conversation
## What changed? priTaskReader.setReadLevelAfterGap now advances the ack level to the new read level whenever outstandingTasks is empty, instead of only when ackLevel == readLevel. Added a getTasksHook to testTaskManager, a holdReadsUntilAcked option to testSkipExpiredTasks, and a new case, TestSkipExpiredTasks_ValidThenExpired_AckedBeforeExpiredRead. Un-skipped TestSkipExpiredTasks_AllExpired for the pri reader. ## Why? Fixes temporalio#12472. processTaskBatch drops expired tasks when it reads them, so they never enter outstandingTasks. If the last outstanding task was acked before the reader read a trailing run of expired tasks, setReadLevelAfterGap saw ackLevel != readLevel and left the ack level stuck at the last acked task. The expired tasks were not GC'd and the backlog count was not reset until another task was written and acked. This also made TestSkipExpiredTasks_ValidThenExpired flaky under load. An empty outstandingTasks means every task up to readLevel was acked or dropped as expired, so acking up to the new read level is safe. ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks The ack level can now advance in setReadLevelAfterGap in cases where it previously did not. This relies on every task at or below readLevel being in outstandingTasks or dropped as expired when read, which is how processTaskBatch works today.
|
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
priTaskReader.setReadLevelAfterGapnow advances the ack level to the new read level wheneveroutstandingTasksis empty, instead of only whenackLevel == readLevel.Tests:
getTasksHooktotestTaskManagerso tests can control read timing.holdReadsUntilAckedoption totestSkipExpiredTasksand a new case,TestSkipExpiredTasks_ValidThenExpired_AckedBeforeExpiredRead, which forces the reader to ack the valid tasks before it reads the trailing expired ones.TestSkipExpiredTasks_AllExpiredfor the pri reader.Why?
Fixes #12472.
processTaskBatchdrops expired tasks as it reads them, so they never enteroutstandingTasks. Suppose the last outstanding task is acked before the reader reaches a trailing run of expired tasks. ThenackTaskLockeddoesn't jump the ack level, because the reader isn't drained yet. Later,setReadLevelAfterGapseesackLevel != readLeveland does nothing. The ack level stays stuck until another task is written and acked. Meanwhile the expired tasks aren't GC'd, the backlog count isn't reset, and a reload re-reads from the stale ack level. This is also what makesTestSkipExpiredTasks_ValidThenExpiredflaky under load.An empty
outstandingTasksmeans every task up toreadLevelwas either acked or dropped as expired, so it's safe to ack up to the new read level. That covers the existing idle-queue case and the expired-tasks case.How did you test it?
On
main, the new test and the un-skippedTestSkipExpiredTasks_AllExpiredfail 5/5 for the pri reader, and both pass with this change.TestSkipExpiredTasks*andTestExpiredTasksOnRead*(Classic/Pri/Fair) pass ×20 with-race, andgo test -race ./service/matching/...passes.Potential risks
The ack level can now advance in
setReadLevelAfterGapin cases where it previously didn't. That relies on every task at or belowreadLevelbeing either inoutstandingTasksor dropped as expired when read, which is howprocessTaskBatchworks today. The stale-read guard (newReadLevel < tr.readLevel) is unchanged.