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

New subsystem API #221

Merged
merged 7 commits into from
Apr 8, 2019
Merged

New subsystem API #221

merged 7 commits into from
Apr 8, 2019

Conversation

pathunstrom
Copy link
Collaborator

@pathunstrom pathunstrom commented Apr 1, 2019

So, doing what I discussed in the slack: Removing activate, adding a heartbeat event.

Need to decide if we heartbeat between each publish, or we clear the queue between each heartbeat.

If we do the former, we're going to start seeing scheduling silliness, but that just comes from asynchronous design.

If we do the latter, it's easier to make events happen in a sequence, but still going to have some of the asynchronous silliness because, well, we're technically async.

  • Add heartbeat, switch the test utilities to use a heartbeat system.
  • Remove activate, switch remaining systems to use heartbeat system.
  • Clean up the main loop to reduce the processing time of the busy loop.

@AstraLuma
Copy link
Member

This is meant as an Idle event? Could it be an event that's triggered not through the signal() path but by the event loop itself?

@AstraLuma
Copy link
Member

Idle implies "when the queue is empty"

@pathunstrom
Copy link
Collaborator Author

That's the question in the beginning, is it better as an idle event or as simply a timing event in between?

It needs to be signaled currently because publish does the popping of the queue.

ppb/events.py Show resolved Hide resolved
@pathunstrom
Copy link
Collaborator Author

I guess, as a followup, it doesn't have to be the way it is now, but that's. . . at least two steps forward in the process. This work gets the heartbeat working and updates Failer to use the thing. The next step would be removing activate and updating the rest of the systems. From there, we can change how publish works and move the pop into the main loop, and let it publish all before the next heartbeat.

So. . . summary:

This is the first (workable) step of a series:

  1. Add heartbeat, switch the test utilities to use a heartbeat system.
  2. Remove activate, switch remaining systems to use heartbeat system.
  3. Clean up the main loop to reduce the processing time of the busy loop.

Theoretically, we can break it up further, but I think this is the right amount of breaking up to be able to properly test things in the interim.

@pathunstrom pathunstrom requested a review from a team as a code owner April 7, 2019 23:18
@pathunstrom pathunstrom changed the title [WIP] New subsystem API New subsystem API Apr 8, 2019
@pathunstrom
Copy link
Collaborator Author

I think this one is good for review.

Copy link
Member

@AstraLuma AstraLuma left a comment

Choose a reason for hiding this comment

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

Could you remove ppb/features/__init__.py? It's not necessary and I'm handling that in the animation PR.

ppb/engine.py Outdated Show resolved Hide resolved
ppb/systems/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@AstraLuma AstraLuma left a comment

Choose a reason for hiding this comment

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

lgtm

@AstraLuma
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Apr 8, 2019
221: New subsystem API r=astronouth7303 a=pathunstrom

So, doing what I discussed in the slack: Removing activate, adding a heartbeat event.

Need to decide if we heartbeat between each publish, or we clear the queue between each heartbeat.

If we do the former, we're going to start seeing scheduling silliness, but that just comes from asynchronous design.

If we do the latter, it's easier to make events happen in a sequence, but still going to have some of the asynchronous silliness because, well, we're technically async.

- [x] Add heartbeat, switch the test utilities to use a heartbeat system.
- [x] Remove activate, switch remaining systems to use heartbeat system.
- [x] Clean up the main loop to reduce the processing time of the busy loop.

Co-authored-by: Piper Thunstrom <pathunstrom@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 8, 2019

Build succeeded

  • dockerfiles
  • docs
  • Linux Dockerfile:.ci/dockerfiles/python_3.6-slim
  • Linux Dockerfile:.ci/dockerfiles/python_3.7-slim
  • macOS PYTHON:3.6.8
  • macOS PYTHON:3.7.2
  • Windows Dockerfile:.ci/dockerfiles/python_3.6-windowsservercore-1809
  • Windows Dockerfile:.ci/dockerfiles/python_3.7-windowsservercore-1809

@bors bors bot merged commit 42d4242 into ppb:master Apr 8, 2019
@pathunstrom pathunstrom deleted the new_subsystem_api branch October 24, 2020 20:54
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