Skip to content

Recorder async & remove start_recording event#6167

Merged
balloob merged 2 commits into
home-assistant:devfrom
kellerza:recorder_timer
Feb 25, 2017
Merged

Recorder async & remove start_recording event#6167
balloob merged 2 commits into
home-assistant:devfrom
kellerza:recorder_timer

Conversation

@kellerza
Copy link
Copy Markdown
Member

@kellerza kellerza commented Feb 22, 2017

Description:
#4614 made the startup timer for recorder 30 seconds - way too short for certain components.

This PR removes the recording_start Event that in the past indicated the recorder can start writing. Since the restore cache inspects the run, we can really start writing data in the new run earlier.

In order to check if the recorder connected in an async friendly way (without blocking the executor pool) this PR adds an asyncio.Event async_db_ready which is equivalent to the threading.Event db_event.
These events are checked with get_instance and async_get_instance respectively.

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

@kellerza, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rhooper, @balloob and @armills to be potential reviewers.

@lwis
Copy link
Copy Markdown
Member

lwis commented Feb 22, 2017

Not sure if you saw in Gitter but we briefly discussed driving this off events instead. But this will fix the immediate issue.

@kellerza
Copy link
Copy Markdown
Member Author

@lwis not sure what your timeframe is to implement events, but @balloob would like to pin the release today still

Not sure how you'd do the events either

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 22, 2017

In recorder:

On Home Assistant start, setup connection:

hass.bus.async_listen(EVENT_HOMEASSISTANT_START, self._setup_connection)

Add this to _setup_connection() after connected:

hass.bus.async_fire(EVENT_RECORDER_CONNECTED)

Only start the recorder thread when the event fires. Same for anything that wants to wait for recorder to be connected.

hass.bus.listen_once(EVENT_RECORDER_CONNECTED, thread.start)

@kellerza
Copy link
Copy Markdown
Member Author

The connection needs to be setup before hass starts, else restore will not work. For restore to work, you also need unfinished runs to be closed.
While the connection sets up we also want to have the capability to block another thread while waiting for the connection (could probably check started, but threading.Event has a nice "wait" function with a timeout.

Not saying it wont work, but probably needs a bit more thought before we include this (and rather not 1/2 days before release)

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Feb 22, 2017

I think recorder going to a very importent part of the eco system and should setup very soon after start and we should wait until we can connect or a timeout will reach. I think a data layer will be more importent in the future for smart homes and here cognitive features.

The restore feature should grip on bootstrap befor homeassistant will start with his EVENT_HOMEASSISTANT_START.

Copy link
Copy Markdown
Member Author

@kellerza kellerza Feb 23, 2017

Choose a reason for hiding this comment

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

After adding some "blocking" components like icloud, I think we should make this default even longer, maybe 30 seconds

Maybe not, the event I get is "Waiting to start recording".....

All OK then

@kellerza kellerza added this to the 0.39 milestone Feb 23, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

@kellerza if it needs to be connected before start, we can add a helper to Recorder:

@callback
def on_connect(hass, callback):
    if is_connected:
        callback()

    @callback
    def recorder_started(event):
        callback()

    hass.bus.async_listen(RECORDER_CONNECTED, recorder_started)

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

The problem with locking threads is that we only have 10… for everything. Having 5 threads be blocked means that HASS is running at 50% performance for threaded platforms/components.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

Ah, but a problem that we have is that startup is fast (like it should be), restoring the state will not be allowed because hass.state == CoreState.running

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

Maybe we should only clear the cache a minute after starting up?

@kellerza
Copy link
Copy Markdown
Member Author

Ok, let me add asyncio.Event to play nicely with async world (so you can test before you submit DB work to Execuror). This will solve filling the restore cache blocking, but the issue this PR adresses is quite different:

Recorder has its own thread. This thread now only waits 30s from start (being the first component to be called) to HASS_START, which is not enough for things that blocks during startup, so we can:

  • make this time longer (this PR)
  • The other alternative would be not to block starting writing to the DB (restore_state should be ok with this since it inspects the run)

@kellerza
Copy link
Copy Markdown
Member Author

Clearing cache later can be done, but also solves other issues, not related to this PR

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

Instead of doing all these workarounds, I have a better proposal. More impactful, more better:

Do the connection inside setup(). Since Home Assistant sets up the recorder first, all other components will have to wait till connection is made and we can remove all these hacks.

Since startup depends on a DB connection now, I think that it only makes sense?

setup should also fail when a connection cannot be made, what do you think?

@balloob balloob mentioned this pull request Feb 24, 2017
2 tasks
@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 24, 2017

What do you think of this fix? #6196

@kellerza kellerza changed the title Allow longer startup time for recorder Recorder async & remove start_recording event Feb 25, 2017
@balloob balloob merged commit ecd1da6 into home-assistant:dev Feb 25, 2017
@balloob balloob mentioned this pull request Feb 26, 2017
2 tasks
@kellerza kellerza deleted the recorder_timer branch February 28, 2017 20:09
@home-assistant home-assistant locked and limited conversation to collaborators Jun 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants