Tried above snippet with this fix applied (snapshot #3299) and got
2023-02-02 09:45:15.395 [WARN ] [nhab.automation.script.ui.8e9939533b] - changedSince true: true
2023-02-02 09:45:15.406 [WARN ] [nhab.automation.script.ui.8e9939533b] - changedSince false: false
The reason why I think it’s a problem in core is @Dan saw the behavior with InfluxDB and I saw it with MapDB which implies something is going on in the Persistence code, not the add-on code.
Yes, it doesn’t do anything itself but wrap the call to the Java Persistence Actions. This is another reason why I identified core as the source.
The exception went away for me now with rrd4j on #3299 
but I’m not getting
2023-02-02 11:53:04.985 [INFO ] [nhab.automation.script.ui.scratchpad] - changedSince true: false
2023-02-02 11:53:05.102 [INFO ] [nhab.automation.script.ui.scratchpad] - changedSince false: false
???
Note, I am passing the DB I want to query now so the code is
var item = items.getItem('TestSwitch');
item.postUpdate( ((item.state == "ON") ? "OFF" : "ON") );
setTimeout( () => {
console.log('changedSince true: ' + items.getItem('TestSwitch').history.changedSince(time.toZDT('PT-1M'), 'rrd4j'));
console.log('changedSince false: ' + items.getItem('TestSwitch').history.changedSince(time.toZDT('PT-30S'), 'rrd4j'));
}, time.toZDT('PT50S').getMillisFromNow());
MapDB gives
2023-02-02 11:58:46.326 [INFO ] [nhab.automation.script.ui.scratchpad] - changedSince true: true
2023-02-02 11:58:46.332 [INFO ] [nhab.automation.script.ui.scratchpad] - changedSince false: true
I think MapDB can’t properly handle changedSince because it does only store one value.
I’m not sure if the binning of rrd4j makes changedSince unusable.
For InfluxDB it works perfectly fine for me. All other services should work.
But it knows when that one value was stored. If it assumes an everyChange strategy, which I think is the default, than if the entry is before the passed in time changedSince would be false and if after changedSince would be true.
That would be odd I think. Surely the binning would impact the ability to see the records between now and the passed in time to see if the state changed in that time? Unless you are going back a long way (days or more) the binning shouldn’t come into play. IIRC the first compression happens at least one day, maybe three days. So it should work at least for short times back (I’m using one minute above).
There was also a bug which should be fixed by Fix wrong .historicState call in .changedSince by J-N-K · Pull Request #3348 · openhab/openhab-core · GitHub.
InfluxDB heaves the same as before (correct).
MapDB now returns false in both cases, which is also the correct behavior in my opinion. We simply can’t know what the state before the stored value is, it could well be the same as the stored valued. Also MapDB doesn’t honor the query filter, it just returns the only value available.
Given that argument, lastUpdate in rrd4j is wrong because we can’t know if the most recent value in the database is because of an update or everyMinute.
Though I would argue that lastUpdate is broken across the board because we can’t know what strategies are configured and if there is any time based persistence we can’t know what we are getting.
To me, to make lastUpdate usable we have to make the same kinda of assumptions about the persistence strategies deployed that you say disqualify changedSince from working for MapDB.
I can buy the filter arrangement as meaning that MapDB can’t conform to the expected API (I guess, im not sure what you really mean by doesn’t honor the query filter and how that relates). But then shouldn’t it return null or throw an error instead of returning a misleading answer?
Last update, previousState and changedSince should just be taken out of persistence and be embedded into GenericItem class
That just moves the problem to restoreOnStartup, but the problem remains, unless we are OK with breaking the use case where we care about updates on very slowly updated Items (once a week for example).
I think it should be just be made clear that not all extension methods are useful on all persistence services. averageSince doesn’t make sense for MapDB.
From my understanding rrd4j always reduces data. We define as default:
// 1. granularity of 10s for the last hour
// 2. granularity of 1m for the last week
// 3. granularity of 15m for the last year
// 4. granularity of 1h for the last 5 years
// 5. granularity of 1d for the last 10 years
So if you update every second, and request now-2s every second, you’ll already see the effect (unfortunately my test rule only did that for about 10s and then ran unto an IllegalMonitorStateException which is probably a bug in rrd4j).
That doesn’t change the point I’m trying to get across though. No matter how it’s working, what we are saying is that lastUpdate for rrd4j will never be longer ago than 10 seconds which is equally as meaningless as trying to call averageSince on MapDB. It’s never going to be the case where lastUpdate on rrd4j will have a meaningful answer. It will always just be when rrd4j saved a record to keep the compression.
That’s the point I’m trying to make. If it doesn’t make sense to have changedSince on MapDB, the same logic should apply to lastUpdate for rrd4j. Though there is one possible way where changedSince could make sense if the MapDB persistence strategy is set to everyUpdate.
For the other databases it’s at least possible to configure the persistence where lastUpdate makes sense but only for certain persistence strategies, but that means the logic that says changedSince shouldn’t be allowed for MapDB would apply to all the databases because we can never know whether the user has applied an everyMinute strategy.
It’s the inconsistency that’s bugging me. It gets complicated when we apply different criteria to different databases instead of setting a rule that applies to all.
IMO we provide a set of persistence extensions that can be used. If a database does not properly support these methods (which are in fact only “convenience” methods), this should be documented in the add-on. Removing these methods from the persistence extensions just because some don’t (fully) support them feels wrong, because as far as I can see only RRD4J and MapDB have limitations.
Ok, thank you all! I think this thread can be marked as solved now…
I’m definitely not arguing to remove these methods at all.
I’m arguing that either we assume that the end user knows what they are doing (with docs) in which case, it is possible for MapDB to return a correct value with changedSince if the persistence strategy is only everyChange. So either MapDB is hard coded to always return false or there is a bug.
If it’s hard coded to false, I’m arguing it should be hard coded to null or it should throw an exception. If not, then it should work. Have MapDB assume that the record represents a change and report true if the timestamp is after the passed in timestamp.
And I extend the same reasoning to lastUpdate. It is possible to configure any of the databases where lastUpdate gives a not meaningful answer but it isn’t possible to configure rrd4j to provide a meaningful answer. So either it should hard code null or it should throw an error.
In both cases, allowing it to return an incorrect answer is confusing and misleading.
The problem is that the implementations are in PersistenceExtensions.java which queries the service for “all states since x”, “all states between x and y” or "state at ". The service does not know what the caller wants to do with the information.
Example:
changedSince(x)
At what point should the persistence service (which only knows about the queries) return something like “I can’t do that”?
I assumed that the processing of the results happened in the add-on, not core. If it’s core than yes, I agree that there probably is no way for the add-on to know the purpose of the query and therefore can only do the best it can.
However, I’m still hung up on chanedSince (though I’m willing to let it go). If the add-on returns only one value, I wonder if it makes more sense to return null rather than to assert that it didn’t change just because we can’t know if it changed.
I’m not sure either. But as you pointed out here, we also have very slowly changing items. These may have valid one-datapoint query results.