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

Remove internal retry from AddTask #6200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Shaddoll
Copy link
Contributor

@Shaddoll Shaddoll commented Jul 30, 2024

What changed?

  • Remove retry logic from AddTask
  • Add wait time for writing task to database if sync match fails

Why?

  • The retry logic is not optimal for AddTask function. It spends a short time on sync match without waiting for a poller and then another short time waiting for the tasks to be written to database, and it retries if these 2 steps fails. But if the retry succeeds on sync match, it means time is wasted on waiting for tasks to be written to database. Thus, we should allocate some time for sync match first and then some time for writing tasks to database.

How did you test it?
unit tests, integration tests

Potential risks
AddActivityTask/AddDecisionTask availability may drop, and task matching latency can increase. But we can rollback the change if that happens.

Release notes

Documentation Changes

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 55.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.91%. Comparing base (f3350d0) to head (3e06d67).
Report is 4 commits behind head on master.

Files Patch % Lines
service/matching/tasklist/task_list_manager.go 66.66% 3 Missing and 2 partials ⚠️
service/matching/tasklist/task_reader.go 0.00% 2 Missing ⚠️
service/matching/tasklist/task_writer.go 33.33% 2 Missing ⚠️
Additional details and impacted files
Files Coverage Δ
service/matching/tasklist/task_reader.go 73.88% <0.00%> (ø)
service/matching/tasklist/task_writer.go 77.44% <33.33%> (ø)
service/matching/tasklist/task_list_manager.go 64.62% <66.66%> (+0.55%) ⬆️

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3350d0...3e06d67. Read the comment docs.


e.EventName = "Task Sent to Writer"
if params.ForwardedFrom != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would it be possible to put this into a method like params.IsForwarded() so that it's clearer what this check is indicating?

Copy link
Contributor

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

minor nit, but otherwise LGTM, we do need to simplify this flow a lot, so this seems like the right direction to me

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