Persistence extensions, add lastChange and nextChange #4259
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/persistence-on-off-transitions-with-influx/156271/7 |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@mherwege could you please apply spotless? |
|
@holgerfriedrich Sorry about that. It should be fine now. |
| int itemCount = 1; | ||
| if (!historicItem.getState().equals(state)) { | ||
| // Stored value different from current state, use now as timestamp when looking backward | ||
| return forward ? historicItem.getTimestamp() : ZonedDateTime.now(); |
There was a problem hiding this comment.
Why do you return the current time? I would say that the expected behavior would be to get the "next change" when forward is enabled.
There was a problem hiding this comment.
I believe that is what it does. If looking forward (nextChange) it will return the next change. If looking backward (lastChange), it will return now when the current state is different from the first one it finds in history.
There was a problem hiding this comment.
If looking backward (lastChange), it will return now when the current state is different from the first one it finds in history.
I would've expected it to return the time when the change occurred, instead of the current time.
There was a problem hiding this comment.
If looking backward (lastChange), it will return now when the current state is different from the first one it finds in history.
I would've expected it to return the time when the change occurred, instead of the current time.
That’s the whole point here. If the change just occurred, it may be it was not written to persistence yet. In that case, now is the best approximation. As long as the state in persistence is the same as the current change, it will keep on looking further back in history.
There was a problem hiding this comment.
This makes me think some similar check may have to be done for last update. If current state different from last state, we know it has been updated since and may return now. That’s incomplete of course because we cannot detect if it has been updated and not stored yet.
There was a problem hiding this comment.
What if the change occurred 40 minutes ago, but the user only has a hourly cron persistence schedule? It's the user's fault for not having
everyChangepolicy in this case but returningnowdoesn't highlight that issue.
A user may have decided hour resolution is good enough for his use case. Returning null would mean we refuse to answer the simple question of when the last change happend within hour resolution.
if you look at the persistence extensions in general, there are various places where we use the current state with timestamp now to return a reasonable answer. Have al look at all the minimum, maximum and deviations methods where we even construct a new historic item with current timestamp. Not doing it here to solve something which is an obvious functional lack (not being able totell since when we have the current state) looks funny to me.
There was a problem hiding this comment.
Returning null would mean we refuse to answer the simple question of when the last change happend within hour resolution.
We should indeed refuse to answer because we (the persistence extension, which provides this method) do not have the requested information. The caller of this method relies on extracting the information that is stored in the persistence database, and not some made up data. Returning now will cause confusion because that doesn't match what's stored in the persistence database.
It should be easy enough, hopefully, for the user script to say lastChange = PersistenceExtensions.lastChange(item) || ZonedDateTime.now() (syntax subject to the language used) if that were the intention of the code.
If this is a common scenario, it can be documented in the JavaDoc if necessary
There was a problem hiding this comment.
Let's make an example of what happens if we take the current value into account and return now.
The item has a "every five minutes" startegy configured. The last persisted value has a timestamp 10:00:00 (and did change from previous value).
10:01:10 lastChange returns 10:00:00
10:02:10 lastChange returns 10:00:00
10:02:20 value is changed
10:03:10 lastChange returns 10:03:10
10:04:10 lastChange returns 10:04:10
10:05:00 value is persisted
10:05:10 lastChange returns 10:05:00
10:06:10 lastChange returns 10:05:00
The value did not change since 10:02:20, but the persistence extension returns three different values for the same change. This looks confusing.
There was a problem hiding this comment.
I know that, and I actually prefer this inconsistency over any other solution. Bu I get the consensus is to rather return null when the current state is different from the last persisted state. If that’s the preference so be it. It is not mine, but I will adapt.
Any return value will always depend on the logic, persistence strategy and specific behavior of the persistence service. If you continue your line of thinking, the persistence extensions should also return null for any since calculation when the current state is different from the last state. You never know exactly what happened between two persisted states. Calculations could actually only be allowed on a service that does not aggregate over time (so not on rrd4j) and only when there is an every change persistence strategy. All else is wrong anyway. And as data gets aggregated over time, it gets worse. We always accepted that. So while I think it is very inconsistent not to allow this approximation and gives a worse user experience, I will accept and adapt the code and javadoc.
| if (!nextHistoricItem.getState().equals(state)) { | ||
| return forward ? nextHistoricItem.getTimestamp() : historicItem.getTimestamp(); |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
| // Past stored value different from current value, so it must have updated since last persist. | ||
| return ZonedDateTime.now(); | ||
| // Last persisted state value different from current state value, so it must have updated since | ||
| // last persist. We do not know when. |
There was a problem hiding this comment.
All warn logging in the persistence extensions so far point to a configuration issue. This is not a configuration issue. I don't think we should pollute the log with warning messages for this. The method javadoc is clear about it.
|
Nothing further from me, but I haven't reviewed this from a maintainer's perspective. I'm happy with the null return change. |
|
Documentation PR is created. @florian-h05 Is there a chance to add this to the javacripting library before 4.2 ? |
Adds support for openhab/openhab-core#4259. Also-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Mark Herwege <mherwege@users.noreply.github.com>
Sure, I've just merged your PR, will now release a new version of the library and then update the add-on. |
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Depends on: openhab/openhab-core#4259 Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/lastupdate-and-null-values/158134/6 |
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Adds support for openhab/openhab-core#4259. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Persistence extensions have lastUpdate and nextUpdate methods. However, when the same value is stored multiple times in the database (e.g. with a everyMinute persistence policy), it doesn't say anything about when the lastChange was.
This PR adds lastChange and nextChange methods.
To further extend on the situation:
Imagine the following persisted values at timestamps 1 to 4 (4 is most recent):
Currently existing persistence methods will return following timestamps:
previousState.timestamp: 4previousState(skipEqual = true).timestamp: 2lastUpdate: 4This enhancement adds a
lastChangemethod which will return timestamp 3, somethings required if one wants to know when an item last changed. All existing methods also suffer from the impact of a persistence strategy. In the example above, if the strategy would have beeneveryChange, and not e.g.everyMinute, timestamps 2 and 4 would not have existed and the existing methods would have given different results.Depending on the persistence strategy and sequence of events, it is possible the current state is not equal to the last persisted state. In that case, the new
lastChangemethod will returnnullas no precise change timestamp can be established. It is the users responsability to then take appropriate actions (e.g. check fornulland usenowwhen calling thelastChangemethod). In the same logic, the existinglastUpdatemethod has been adjusted to also returnnullin that case.A
nextChangemethod was added to have a symetric method, but is not impacted by the reasoning above. Existing methods would yield the expected results already.