Fix history charts not auto refreshing with cached history #12873
Conversation
bdraco
force-pushed
the
fix_updates_cache_history
branch
from
d823457
to
9c2f9b5
Compare
| if (stateHistory.timeline.length) { | ||
| mergeTimeline(stateHistory.timeline, cache.data.timeline); | ||
| // Replace the timeline array to force an update | ||
| cache.data.timeline = [...cache.data.timeline]; |
There was a problem hiding this comment.
Since mutating the array doesn't trigger the update I did this to force it. Is there a better way?
There was a problem hiding this comment.
I would expect mergeLine and mergeTimeline to create new arrays.
There was a problem hiding this comment.
cache.data.timeline = mergeTimeline(stateHistory.timeline, cache.data.timeline);
There was a problem hiding this comment.
That didn't work out very well unfortunately since the update is a partial update and the current code enumerates the new data so if the entity didn't update in history call it disappears.
There was a problem hiding this comment.
I ended up reverting the last commit because I think its too risky to refactor without adding a whole lot of tests.
| @@ -191,6 +197,8 @@ const mergeLine = ( | |||
| oldLine.data.push(entity); | |||
| } | |||
| }); | |||
| // Replace the cached line data to force an update | |||
| oldLine.data = [...oldLine.data]; | |||
There was a problem hiding this comment.
Since mutating the array doesn't trigger the update I did this to force it. Is there a better way?
There was a problem hiding this comment.
This one is even more complex since its arrays in arrays and it has the same issue of only enumerating the new data
bdraco
changed the title
bdraco
force-pushed
the
fix_updates_cache_history
branch
from
9c2f9b5
to
8a60afc
Compare
bdraco
force-pushed
the
fix_updates_cache_history
branch
from
8a60afc
to
2b0e265
Compare
This reverts commit 2b0e265.
|
Not sure how to proceed on this one. I reverted the last commit since the changes hit my risk tolerance for a bug fix as caching is so easy to get wrong. There isn't tests for these so I didn't feel comfortable doing a lot of refactoring here since this one is non trivial to test and would be easy to miss a new bug. |
|
meh looking at this code and it's indeed confusing. Do we even need caching ? It seems like if we are showing a more-info, we always want to show latest, and if a user is bouncing between tabs it can be locally cached, not at the top-level. |
|
I think we only need caching for the case where they have a whole page of history graph cards which I'm learning is apparently a more common use case than expected. 😢 Without caching we would be hammering the backend with requests for 24 hours of history data every time there is a state change for an entity that is being graphed instead of just the new data. |
Proposed change
The git archaeology on this is very difficult to sort out. Best I can tell: while fixing the history caching in #12788 made the cache work now (so we aren't constantly overloading the backend with history requests), it also re-introduced a regression similar to #2061 . I'm not sure that original issue was completely fixed, but the working theory is that since the history cache was no longer effective after #7072 it had the effect of solving it and fixing the history cache reintroduced the regression.
Fixes #12859
Unrelated to this PR, but I think
getRecent(notgetRecentWithCache) is dead code AFAICT.Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: