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

Refactor config entry storage and index #107590

Merged
merged 12 commits into from
Jan 13, 2024
Merged

Refactor config entry storage and index #107590

merged 12 commits into from
Jan 13, 2024

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jan 8, 2024

needs test fixes

Proposed change

Refactors config entry storage to work the same as the entity registry and the device registry. This abstracts all the maintaining of the index into the ConfigEntryItems class.

We now have an index for the unique id as well since this each discovery will have to lookup to see if the unique id already exists. This starts to be a problem when the a device is discovered frequently. An example case of this is when the user has a lot of ESPHome devices and deep sleep configured. This means devices are constantly being rediscovered and since there was previously no index on unique id, it meant all the config entries in the domain had to be enumerated every time a device woke up. In this case we would have actually had to enumerate the whole list twice, with once for async_set_unique_id and once for _abort_if_unique_id_configured

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Refactors config entry storage to work the same as the
entity registry and the device registry. This abstracts
all the maintaining of the index into the ConfigEntryItems
class.

We now have an index for the unique id as well since this
each discovery will have to lookup to see if the unique
id already exists. This starts to be a problem when the
a device is discovered frequently. An example case of this
is when the user has a lot of ESPHome devices and deep
sleep configured. This means devices are constantly being
rediscovered and since there was previously no index on
unique id, it meant all the config entries in the domain
had to be enumerated every time a device woke up
@bdraco
Copy link
Member Author

bdraco commented Jan 10, 2024

running well on my system for a bit. solves the constant unique id lookup from discovery churn issue

@bdraco bdraco marked this pull request as ready for review January 10, 2024 01:04
@bdraco bdraco requested a review from a team as a code owner January 10, 2024 01:04
Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

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

Looks nice! Some minor ideas, but not required.

homeassistant/config_entries.py Outdated Show resolved Hide resolved
homeassistant/config_entries.py Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Jan 13, 2024

Thanks

@bdraco
Copy link
Member Author

bdraco commented Jan 13, 2024

Well all the test failure are bugs in the tests.

I'm going to make it a warning an error for now

@bdraco
Copy link
Member Author

bdraco commented Jan 13, 2024

I will fix them all in a separate PRs

@bdraco
Copy link
Member Author

bdraco commented Jan 13, 2024

I fixed the duplicate adds in everything but plex and zha in #107984

plex and zha will need their own PRs as their logic is a bit more complex

@bdraco bdraco merged commit 3649cb9 into dev Jan 13, 2024
49 checks passed
@bdraco bdraco deleted the config_entry_storage branch January 13, 2024 20:34
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2024
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.

3 participants