Persistence timeseries fix updating Item state#5403
Persistence timeseries fix updating Item state#5403holgerfriedrich merged 20 commits intoopenhab:mainfrom
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
when persisting or removing Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
timeseries Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
manager Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Show resolved
Hide resolved
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Outdated
Show resolved
Hide resolved
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Outdated
Show resolved
Hide resolved
...persistence/src/main/java/org/openhab/core/persistence/extensions/PersistenceExtensions.java
Show resolved
Hide resolved
...istence/src/test/java/org/openhab/core/persistence/extensions/PersistenceExtensionsTest.java
Outdated
Show resolved
Hide resolved
....persistence/src/test/java/org/openhab/core/persistence/internal/PersistenceManagerTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Show resolved
Hide resolved
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void handleExternalPersistenceDataChange(PersistenceService persistenceService, Item item) { | ||
| if (!(persistenceService instanceof QueryablePersistenceService)) { | ||
| return; | ||
| } | ||
| persistenceServiceContainers.values().stream() | ||
| .filter(container -> container.persistenceService.equals(persistenceService) && container | ||
| .getMatchingConfigurations(FORECAST).anyMatch(itemConf -> appliesToItem(itemConf, item))) | ||
| .forEach(container -> container.scheduleNextPersistedForecastForItem(item.getName())); | ||
| .filter(container -> container.persistenceService.equals(persistenceService) && Stream | ||
| .concat(container.getMatchingConfigurations(UPDATE), | ||
| container.getMatchingConfigurations(FORECAST)) | ||
| .distinct().anyMatch(itemConf -> appliesToItem(itemConf, item))) | ||
| .forEach(container -> { | ||
| container.scheduleNextPersistedForecastForItem(item.getName()); | ||
| PersistedItem persistedItem = container.getPersistedItem(item); | ||
| if (persistedItem != null) { | ||
| container.restoreItemState(item, persistedItem); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The handleExternalPersistenceDataChange method has been significantly expanded (now includes getPersistedItem query and restoreItemState) but there are no tests for it in PersistenceManagerTest. Consider adding a test that verifies:
- The early return for non-queryable persistence services
- The item state restoration behavior when external persistence data changes
- The forecast scheduling after an external data change
The PersistenceExtensionsTest only verifies that handleExternalPersistenceDataChange is called on the mock, but doesn't verify the internal behavior of the method itself.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
....openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceManager.java
Outdated
Show resolved
Hide resolved
....persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #4462
Closes #4662
It has been reported a few time that a timeSeries did not update the Item state when passing the timeSeries entry timestamp.
The issue is that persisting a timeSeries from the persistence extension persist method did not get the forecast job running. As long as there were future values in the timeSeries, updating the timeSeries would keep the logic going. But once the timeSeries runs out, a new timeSeries provided through persistence extensions did not start the job anymore. Restarting or resaving the Item would also solve this and get it running again.
This issue is fixed with this PR.
Second, as noted in the second issue to be closed, loading a timeSeries with values in the past would not update the Item state. Restarting openHAB afterwards would if a restoreOnStartup strategy is configured.
While I think it is a bad idea to blindly set the Item state with values from a timeSeries that is being loaded, there certainly is improvement possible. My proposed solution is: if a timeSeries is loaded with values in the past, look for the last value in the past. If the timestamp of that value is later then the Item's last update timestamp, use the value from the timeSeries to set the Item state. As persistence is not involved in this check (it is only the Item's internal state and timestamps), this is only dependent on the Item's state and not if it is persisted or not.
This makes using timeSeries for updating past values more consistent, and will also make sure the item still gets updated when a timeSeries with forecast data is loaded late, after the previous values ran out.
As a logical consequence of 2, I also implemented logic to respect UPDATE and CHANGE strategies for other persistence services (not the one getting the new FORECAST with elements in the past). If the Item state gets updated because a new past state replaced the (older) state of the item, other persistence services then the one from the FORECAST will get their UPDATE or CHANGE strategy applied and a record will be persisted if needed.
Also see https://community.openhab.org/t/a-few-issue-with-time-series/168678
Also see https://community.openhab.org/t/energy-forecast-binding/168567/30