Skip to content

WIP Purge old data from sqlite db on hass start-up#1681

Merged
infamy merged 4 commits into
home-assistant:devfrom
justyns:purge_old_data
May 15, 2016
Merged

WIP Purge old data from sqlite db on hass start-up#1681
infamy merged 4 commits into
home-assistant:devfrom
justyns:purge_old_data

Conversation

@justyns
Copy link
Copy Markdown
Contributor

@justyns justyns commented Apr 2, 2016

Description:

Each time HASS starts, the recorder component will run _purge_old_data() before going into the event loop. _purge_old_data() will delete records from the events and states tables older than X days. After deleting those records, VACUUM is run on the database to free up disk space taken by the deleted records.

This also adds a purge_days config option to the history component. By default it is set to -1 which means no old records will be deleted.

Let me know if purge_days should be changed to something else. I considered purge_after and a few other similar names, but figured purge_days would be obvious that it's expecting days.

Related issue (if applicable): #1337

Example entry for configuration.yaml (if applicable):

# Enables support for tracking state changes over time.
recorder:
  # Delete events and states older than 2 weeks
  purge_days: 14

Checklist:

If code communicates with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

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.

@justyns
Copy link
Copy Markdown
Contributor Author

justyns commented Apr 2, 2016

pylint question:
R:167, 0: Too many instance attributes (8/7) (too-many-instance-attributes)

Should I add a comment to ignore this, or is there a recommendation for what to do here? I added self.config to the Recorder class to get the purge_days config item.

@justyns justyns changed the title Purge old data from sqlite db on hass start-up WIP Purge old data from sqlite db on hass start-up Apr 2, 2016
Comment thread homeassistant/components/recorder.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The recorder should not be aware of a config existing. Instead, let's pass down the parameter for how many days to purge (or None if not to purge at all)

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 2, 2016

Yes, you're allowed to ignore that pylint warning.

This will require tests to be written.

@infamy
Copy link
Copy Markdown
Contributor

infamy commented Apr 15, 2016

@justyns are you still working on adding test cases? Need a hand with that?

@justyns
Copy link
Copy Markdown
Contributor Author

justyns commented Apr 15, 2016

Thanks @infamy . I started adding tests for this the other day, but ran into a couple issues:

  1. I can modify the config passed to recorder in the setUp function, but if I want to test with purge_days with different values(off and on), should I just create a new TestRecorderPurge class with the different config? I'm not sure if there's a cleaner way to test multiple configs.
  2. I didn't see an obvious existing way to insert states/events with a custom timestamp. Should I modify recorder.record_state and recorder.record_event to accept a date/timestamp parameter, or should I just use a raw sql query to insert a few states manually? I'm guessing the latter would make sense for testing.

I'm planning on trying to finish this this weekend, but I don't really have anything worth keeping at the moment if anyone wanted to jump in on the testing.

edit: ignore the first issue, I was thinking about it wrong.

justyns added 3 commits April 15, 2016 19:54
Issue home-assistant#1337

When purge_days is set under the history component, recorder.py will
delete all events and states that are older than purge_days days ago.

Currently, this is only done once at start up.   A vacuum command is
also run to free up the disk space sqlite would still use after deleting
records.
CONF_PURGE_DAYS so it can be changed easier later.

Use 'recorder' domain instead of 'history' domain.

Pass purge_days config directly into Recorder object instead of passing
the config object around.
@justyns
Copy link
Copy Markdown
Contributor Author

justyns commented Apr 16, 2016

@infamy or @balloob, do these tests look okay? It tests whether purge_days is off or set to a number, and verifies it doesn't delete more than it should. I'll fix the lint errors, and clean it up a little, but wanted to check if there's anything else I should be testing since this is the recorder/history component.

@infamy
Copy link
Copy Markdown
Contributor

infamy commented Apr 16, 2016

@justyns you have a bunch of lint errors in the test. (take a look at the Tavis CI output) they should be easy fixes.

As for the test cases those make sense to me.

The only thing I would add, is a base feature to have it do the clean up every day at midnight, instead of just at startup.

You would need to add the import
from homeassistant.helpers.event import (track_utc_time_change)

And create the event to be called
track_utc_time_change(hass, lambda now: purge_data(), hour=0)

Or something like that. That would allow HA to do the clean up nightly. (only create the event handle if it is enabled of course)

_LOGGER.info("Purging events created before %s", purge_before)
deleted_rows = self.query(
sql_query="DELETE FROM events WHERE created < ?;",
data=(int(purge_before.timestamp()),),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should be able to just put in purge_before and it will be translated.

@balloob
Copy link
Copy Markdown
Member

balloob commented Apr 17, 2016

The tests look great 👍

Added some comments.

@infamy infamy mentioned this pull request May 14, 2016
7 tasks
@infamy infamy merged commit bf3b77e into home-assistant:dev May 15, 2016
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants