Skip to content

Reflect API changes in cache layer #17

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mtanasiewicz
Copy link

@mtanasiewicz mtanasiewicz commented Nov 19, 2023

The goal of this PR is to fix existing bugs in the cache layer:

  1. After a successful update of a resource in the Tech REST API, the API still returns old resource data for some time, with the flag duringChange: true. On the Tech side, the data update process usually takes about 1-5 seconds. In this implementation, this situation has been handled by updating the data in the cache only if the flag in the response is set to false.
  2. After a successful update of a resource in the Tech REST API, the cache itself was not updated. Right after that, home assistant tried to update the entity status, getting old data from the cache, thereby restoring the previous values. Updated data were visible in the system only after refreshing the cache through the API again, which took about 30 seconds according to the default refresh setting. In the current implementation, after a successful update of a REST resource, the cache values are also manually updated, so the data visible in the system are consistent with expectations. In case something goes wrong on the server side, the next cache update will restore the data.

This is the first code written by me in the homeassistant ecosystem, as well as the first code written by me in Python. So please be understanding :)

@mtanasiewicz mtanasiewicz changed the title Aktualizacja cachu po aktualizacji danych w API. Reflect API changes in cache layer Nov 19, 2023
@jontofront
Copy link

o good, I try to test it, while @MichalKrasowski review it

Copy link
Owner

@MichalKrasowski MichalKrasowski left a comment

Choose a reason for hiding this comment

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

Looks great 😃👍

@mtanasiewicz
Copy link
Author

@jontofront Have you had any chance to test it? I've tested in on a whole hotel automation production environment and it works like a charm.

@jontofront
Copy link

Yes I test it works great thanks. But I am using another Fork becouse there are sensors of battery ant Humidy. If you will have time maybe you can imoplement properly in this branch?

@mtanasiewicz
Copy link
Author

I think I'll have some time to spare next week. Let me know where should I submit the PR.

@mtanasiewicz
Copy link
Author

@jontofront where should I submit the PR? You mentioned some other fork before.

@jontofront
Copy link

@mtanasiewicz you can implement it there, I think we must have one stable repo there. I cloned this code from there https://github.com/AkiraJager/tech-controllers, because I needed battery level and humidify

@MichalKrasowski
Copy link
Owner

Guys, I propose to move other changes to separate PRs, instead of running this one indefinitely. What do you say, @jontofront , @mtanasiewicz ?

@jontofront
Copy link

Guys, I propose to move other changes to separate PRs, instead of running this one indefinitely. What do you say, @jontofront , @mtanasiewicz ?

If you mean to another branch I think yes you can do it.
Also I think we can make pre realise tag and test it

@daroga0002
Copy link

@mtanasiewicz @jontofront @MichalKrasowski I dont know what is current status of this fork, but some of issues were addressed under upstream repo https://github.com/mariusz-ostoja-swierczynski/tech-controllers which is now quite activly developed and also addresses some issues mentioned here. Maybe you will be interested about it and add some contribution there.

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.

4 participants