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

Robust Runner #109

Merged
merged 91 commits into from
Apr 21, 2022
Merged

Robust Runner #109

merged 91 commits into from
Apr 21, 2022

Conversation

maxfischer2781
Copy link
Member

@maxfischer2781 maxfischer2781 commented Mar 1, 2022

This PR restructures the concurrency runners to be more robust. Major changes include:

  • Reduce public features of private types.
    • Remove paranoid thread concurrency protections.
  • Switch runner core loops to asyncio from threading
    • This should be more efficient due to less thread switching and waits.
  • Move queueing-before-start mechanism to meta runner.
    • This simplifies the actual runners.
    • Runners now use an async queue internally which avoids busy looping.
  • Wrap main thread asyncio loop instead of creating a new loop

Unittests are down from 15-20s to 2-3s now (which doesn't mean that cobald itself is that much faster.)


Note to reviewers:

The public interface of register_payload/run_payload has stayed intact but the parts used by cobald itself has changed considerably. Since the architecture changed (from threading to asyncio) it is probably not helpful to look just at the diff directly.

As a recap of the current architecture:

  • The user facing entry point is ServiceRunner, which has not changed other than some documentation.
  • The next step is MetaRunner, which now runs an asyncio loop instead of relying on ThreadRunner for concurrency.
    • MetaRunner starts and monitors several BaseRunner instances concurrently.
  • The BaseRunner is the template for actual implementations.

@lgtm-com

This comment was marked as duplicate.

@lgtm-com

This comment was marked as duplicate.

@lgtm-com

This comment was marked as duplicate.

@maxfischer2781
Copy link
Member Author

I think I got all comments covered now. Not sure on renaming MetaRunner, I think that's unfortunate but I really have no better idea.

Ready for the next round, reviewers? ;)

eileen-kuehn
eileen-kuehn previously approved these changes Mar 24, 2022
Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Just a minor one that is not required at all 🥇

src/cobald/daemon/runners/base_runner.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 24, 2022

This pull request fixes 1 alert when merging 68b8369 into b33bb8e - view on LGTM.com

fixed alerts:

  • 1 for Except block handles 'BaseException'

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Great stuff 🎉 Set it free!

@maxfischer2781
Copy link
Member Author

@mschnepf any objections to the new changes? Your last review was invalidated by them.

@maxfischer2781
Copy link
Member Author

@mschnepf I think you accidentally that other PR twice. Can you take another look at this one here?

Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

👍

@maxfischer2781 maxfischer2781 merged commit 81a2ca9 into master Apr 21, 2022
@maxfischer2781 maxfischer2781 added this to the Release 0.13 milestone Aug 16, 2022
@maxfischer2781 maxfischer2781 mentioned this pull request Aug 16, 2022
3 tasks
@maxfischer2781 maxfischer2781 deleted the maintenance/robust_runner branch January 17, 2024 15:02
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.

3 participants