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.

Sign up for GitHub

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

Jump to bottom

prevent task starvation #434

Merged
merged 29 commits into from
Dec 7, 2020
Merged

prevent task starvation #434

merged 29 commits into from
Dec 7, 2020

Conversation

stevenzhu94
Copy link
Contributor

closes #380

@stevenzhu94 stevenzhu94 changed the title Develop prevent task starvation Dec 5, 2020
gavv
gavv reviewed Dec 5, 2020
View reviewed changes
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for PR! Here are some comments.

It seems that the travis in develop is broken, I'll take a look.

src/modules/roc_ctl/task_queue.cpp Outdated Show resolved Hide resolved
src/tests/roc_ctl/test_task_queue.cpp Outdated Show resolved Hide resolved
src/tests/roc_ctl/test_task_queue.cpp Outdated Show resolved Hide resolved
@gavv gavv added contribution A pull-request by someone else except maintainers needs revision Pull request should be revised by its author labels Dec 5, 2020
@gavv
Copy link
Member

gavv commented Dec 5, 2020

It seems that the travis in develop is broken, I'll take a look.

Fixed, please rebase.

@stevenzhu94
changed process_task_ to alternate between ready and sleeping task queue
452b797
@stevenzhu94
added ready_queue to TaskQueue constructor and unit test for starvation
da4547d
@stevenzhu94
made fetch_ready_ boolean local rather than a class property
a869a06
@stevenzhu94
Merge branch 'develop' of https://github.com/stevenzhu94/roc-toolkit …
9ad6747 
…into develop
@stevenzhu94
reimplemented fetch_ready_ flag as a class field and only flip when f…
6d4916a 
…etch from original queue was successful, also changed unit test to check for alternating fetches
@stevenzhu94
fixed fetch_ready_ field name in header file
b547569
@stevenzhu94
reordered fetch_ready_ member declaration in header file
314b3b6
@stevenzhu94
added debugging printouts
c55ae53
@stevenzhu94
fixed printout
918bdf4
@stevenzhu94
fixed test for all sleep test to have same deadline
d4c8d36
@stevenzhu94
increased synchronizing wait time in test
1a58980
@stevenzhu94
changed test
372f92f
@stevenzhu94
update test
b9fec01
@stevenzhu94
update test
b565d46
@stevenzhu94
update test
82b8971
@stevenzhu94
update test
7101d57
@stevenzhu94
update test
91861df
@stevenzhu94
update test
c7ddd2c
@stevenzhu94
update test
7bf2824
@stevenzhu94
update unit test
b553ba9
@stevenzhu94
Copy link
Contributor Author

Had to do some printouts to figure out why the tests were failing. Turns out there were some synchronization issues when creating the tests in a loop, so I changed the test to explicitly schedule and unblock one by one.

@stevenzhu94
Copy link
Contributor Author

the travis bionic builds initially failed the schedule_at_and_schedule unit test, but they passed after I ran them again. The alternating fetch should not affect the test unless the tasks scheduled using schedule_at become expired. I'll increase the delay on those tests and build again.

@gavv
Copy link
Member

gavv commented Dec 7, 2020

LGTM!

@gavv gavv merged commit bb7d974 into roc-streaming:develop Dec 7, 2020
@gavv gavv removed the needs revision Pull request should be revised by its author label Dec 7, 2020
@gavv gavv added this to the 0.2.0 milestone Dec 7, 2020
@gavv
Copy link
Member

gavv commented Dec 7, 2020

the travis bionic builds initially failed the schedule_at_and_schedule unit test, but they passed after I ran them again. The alternating fetch should not affect the test unless the tasks scheduled using schedule_at become expired. I'll increase the delay on those tests and build again.

Thanks. Ideally in future we should use virtual time in tests, I've created an issue for that: #435.

gavv pushed a commit to gavv/roc-toolkit that referenced this pull request Mar 5, 2021
@stevenzhu94 @gavv
prevent task starvation (roc-streaming#434)
df4ee14
gavv pushed a commit to gavv/roc-toolkit that referenced this pull request Mar 5, 2021
@stevenzhu94 @gavv
prevent task starvation (roc-streaming#434)
08be26d
gavv pushed a commit to gavv/roc-toolkit that referenced this pull request Nov 29, 2022
@stevenzhu94 @gavv
prevent task starvation (roc-streaming#434)
95ced04
gavv pushed a commit to gavv/roc-toolkit that referenced this pull request Dec 1, 2022
@stevenzhu94 @gavv
prevent task starvation (roc-streaming#434)
2fc9ef9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavv gavv gavv left review comments

Assignees
No one assigned
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@stevenzhu94 @gavv