Initial commit of persistence extensions #1872

cdjackson · 2016-07-17T18:13:52Z

This contains initial thoughts for persistence extensions.

Two extensions are considered -:

An interface to write an HistoricState. This allows bindings to write data with a timestamp that is not the current time. This is needed to support devices that periodically provide a dump of historic data.
An admin interface that allows the database to be administered. This supports methods to return the current list of items that are persisted, and supports deleting items from the database and deleting data from an item.

I'm sure there are other ideas that can improve this, but hopefully this will help to prompt some discussion...

Signed-off-by: Chris Jackson [email protected]

Signed-off-by: Chris Jackson <[email protected]>

kaikreuzer · 2016-07-19T00:23:01Z

.../src/main/java/org/eclipse/smarthome/core/persistence/HistoricalPersistenceStoreService.java

+ * @author Chris Jackson - Initial implementation and API
+ *
+ */
+public interface HistoricalPersistenceStoreService {


The current interfaces are called PersistenceService and QueryablePersistenceService.
So "Store" is definitely too much here. I also wonder, if there is a better description than "Historical". How about ModifiablePersistenceService?

Should this interface directly extend QueryablePersistenceService or can there be real cases where it isn't queryable?

The PersistenceAdminService also wants to address removal of entries. If I remember our past discussions right, you probably want to add some delete/remove methods to this interface as well, don't you?

So "Store" is definitely too much here.

Ok - HistoricalPersistenceService?

How about ModifiablePersistenceService?

But it's not modifyable - it just allows you to store information with a time - you can't update anything in the store so in this respect it's no different than the other services.

Should this interface directly extend QueryablePersistenceService

Yes - you're probably right - I don't think it makes sense without query.

The PersistenceAdminService also wants to address removal of entries. If I remember our past discussions right, you probably want to add some delete/remove methods to this interface as well, don't you?

Yes - I split those into a separate interface though. I'm happy to consolidate into a single interface if you prefer, but I recall in the past you wanted to keep admin (delete) functions separate.

But it's not modifyable

Right, but if we combine it with your other "separate interface to come", it will be :-)

I'm happy to consolidate into a single interface if you prefer, but I recall in the past you wanted to keep admin (delete) functions separate.

Well, I wanted to keep any non-chonological write operations separate. If we introduce a "store with timestamp" method, I think this anyhow means that the service is able to modify its already persisted data - and hence also offers deletion of entries etc. So for me, it makes sense to combine those methods in one interface.

Right, but if we combine it with your other "separate interface to come", it will be :-)

Yes, if we combine into a single interface, then this is fine with me.

cdjackson · 2016-07-19T09:24:34Z

Ok - so I will update this PR with a single ModifyablePersistenceService interface as above. I'll also look to extend the REST interface to support the extra methods.

I'll also look to see if I can port a single persistence service to ESH to implement these services. This might take a little thought - ideally I would have done the JDBC, but there might be license issues and I might need to break out the drivers into separate repos to make it ESH compatible.

kaikreuzer · 2016-07-19T09:33:03Z

with a single ModifyablePersistenceService interface as above

You are the native speaker, but isn't the correct spelling Modifiable?

I'll also look to see if I can port a single persistence service to ESH to implement these services.

What about your H2 efforts you were once doing? As it is available under EPL, this sounds like a pretty good candidate?

kaikreuzer · 2016-07-19T09:34:16Z

...ersistence/src/main/java/org/eclipse/smarthome/core/persistence/PersistenceAdminService.java

+     * Returns a list of items that are stored in the persistence service
+     *
+     * This is returned as a string to allow the persistence service to return items that are no long available as an
+     * openHAB {@link Item}.


please don't mention openHAB here

cdjackson · 2016-07-19T11:41:42Z

You are the native speaker, but isn't the correct spelling Modifiable?

Native speaker, yes, but also an engineer, so be definition, spelling is far from my strong point! :)

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2016-07-21T21:31:15Z

API updated -:

Changes made to QueryablePersistenceService and ModifiablePersistenceService as discussed
store method changed to add a time as a parameter rather than to use HistoricalItem. This didn't work well since it didn't include the Item - only the item name, and that wasn't ideal as the item type is needed in the persistence service.
getItems method updated to return a custom type PersistenceItem which allows more than just the name to be returned. Currently I have added the item name, and number of rows.

Any thoughts on this appreciated. I've already made most of the mods to the persistence service and added the H2SQL service - more testing and tidying required of course, but before I do I thought I'd get feedback on the API.

kaikreuzer · 2016-07-22T15:58:06Z

...tence/src/main/java/org/eclipse/smarthome/core/persistence/ModifiablePersistenceService.java

+     *
+     * @return true if the item was deleted
+     */
+    public boolean removeItemData(String itemName);


to make it very clear, I would call it removeAllItemData.

I am considering to remove this method and instead just have the remove method. If all data is removed using the remove method, then the item should also be deleted from the database. I think this is a more fail safe method - it means you need to actively configure a filter with a start/stop time that includes all data.

What do you think?

kaikreuzer · 2016-07-22T16:02:10Z

lgtm, so fine for me if you go ahead. @maggu2810 Any comments from your end?

maggu2810 · 2016-07-22T16:17:11Z

...tence/src/main/java/org/eclipse/smarthome/core/persistence/ModifiablePersistenceService.java

+     * @param date the date of the record
+     * @param item the data to be stored
+     */
+    public void store(Date date, Item item);


If you don't care, I would prefer to use Item as first argument and Date as second one.
Why? Because there is already the method void store(Item item);.
https://github.com/eclipse/smarthome/blob/e0985e9/bundles/core/org.eclipse.smarthome.core.persistence/src/main/java/org/eclipse/smarthome/core/persistence/PersistenceService.java#L40
And if this one has an additional argument I prefer to add it at the endt.

We can remove "public" modifiers on interface functions (its default).

I know - the reason I added them was to maintain the same style as other interfaces in this package. I’m happy to remove it though.

cdjackson · 2016-07-22T16:54:35Z

Updated following latest comments. As per previous comment, I've now removed the method to delete the item. It is expected that the persistence service should free resources associated with an item if all data is deleted.

I've also added the REST changes if you have comments here.

H2SQL implementation to follow.

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2016-07-23T10:00:54Z

So, there is another issue with the store(item, time) method. The problem is that you can't use it as you can't set the state of an item, and even if you could, you wouldn't want to set it to an old value. In order to use this method, the implementer would need to create a class that implements Item, and this seems like a lot of unnecessary hassle.

So, options...

Revert to my original idea of using the store(HistoricState) method. This has the problem that persistence services probably want to know the item type (all the SQL related services do at least), and that's not available in HistoricState.
Implement store(item, HistoricState).
Implement store(item, time, state).
Your idea here...

Signed-off-by: Chris Jackson <[email protected]>

maggu2810 · 2016-07-23T17:22:44Z

I assume 2) or 3) should be fine.

maggu2810 · 2016-07-23T17:50:59Z

...re.persistence/src/main/java/org/eclipse/smarthome/core/persistence/PersistenceItemInfo.java

+     *
+     * @return count of the number of rows of data.
+     */
+    Integer getRows();


See: https://github.com/eclipse/smarthome/pull/1872/files/b532fa51461caa8855eedacb3e203fad9b9d5e59#r71975733

cdjackson · 2016-08-02T06:54:50Z

Looks like I missed a reference in bundles/ui/org.eclipse.smarthome.ui/src/main/java/org/eclipse/smarthome/ui/internal/chart/DefaultChartProvider.java. I'll update this tonight.

kaikreuzer · 2016-08-02T08:34:06Z

...tence/src/main/java/org/eclipse/smarthome/core/persistence/ModifiablePersistenceService.java

+     *
+     * @param filter the filter to apply to the data removal. ItemName can not be null.
+     * @return true if the query executed successfully
+     * @throws {@link InvalidParameterException} if item name is null.


Err, no: "This exception, designed for use by the JCA/JCE engine classes".
Better use IllegalArgumentException instead.

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2016-08-02T15:01:47Z

Updated, and hopefully good to go ;)

kaikreuzer · 2016-08-02T15:31:56Z

Thanks, looks all good to me!
The only one still complaining is Travis with a compilation error in TestPersistenceService...

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2016-08-02T16:09:51Z

Sorry- missed that one... Fixed now ;)

kaikreuzer · 2016-08-02T16:11:02Z

Wonderful 👍

* master: Initial commit of persistence extensions (eclipse-archived#1872) fixed concurrency issues (eclipse-archived#1950) Fix usage of normalizer in Automation module. (eclipse-archived#1938) changed log level for config validation (eclipse-archived#1953) Enhancement to hue binding documentation (eclipse-archived#1952) Change the hue binding to include the thing type id in the thingUID again (eclipse-archived#1946)

kaikreuzer · 2016-08-02T21:00:18Z

S%$#t, @cdjackson, I didn't notice that PersistenceExtensions is in smarthome.model, not in smarthome.core, so using it created an invalid dependency, see https://openhab.ci.cloudbees.com/job/openHAB-Core/60/console:

Unable to resolve org.eclipse.smarthome.io.rest.core/0.9.0.201608022010: missing requirement [org.eclipse.smarthome.io.rest.core/0.9.0.201608022010] osgi.wiring.package; filter:="(osgi.wiring.package=org.eclipse.smarthome.model.persistence.extensions)"

So we will need a different solution after all.
How about introducing a core.persistence.PersistenceServiceRegistry, where we can centrally offer methods like providing all services of a certain type, configuring the default, etc.? The other services could then refer to this and it might be useful in future anyhow (as not every service has to track all the different PersistenceServices itself).

cdjackson · 2016-08-03T06:25:26Z

Other than for historical reasons, is there a reason that the persistence extensons are in the model bundle? IMHO, they should probably be part of the core.persistence bundle anyway.

I don't mind adding a registry, but I'm just wondering if we shouldn't do a slightly bigger refactor and move the PersistenceExtensions into the core bundle anyway, and then add the registry in with that? That would keep the model dealing with the model stuff only, and this is probably (??) a closer architecture to other models (like items at least) although I've not looked at them all.

I'm not sure if that would break anything (further!) or not - it just seems to be to be more logical and I suspect if we want to change it, now would be the time?

kaikreuzer · 2016-08-03T06:36:03Z

If you check the history, you will actually notice that it had been in core before and was moved to model on purpose.
The reason was to keep all DSL related stuff in the model bundles and although the PersistenceExtensions is specifically there to provide methods for the DSL rules.
So no, moving it is not an option, especially not as long as it depends on jodatime, which should not be a core part either.

kaikreuzer · 2016-08-03T06:54:32Z

Shall I work on this? It is rather critical as the OH distro build is currently broken by it...

cdjackson · 2016-08-03T07:27:51Z

Ok - I'd assumed that these extensions were also going to be available in other rules as well - ie not just the 'old' DSL rules...

Shall I work on this?

Sure - I can't do anything until later in the day, so if you have the time today then it might be safer (he says, knowing no-one really has 'time' :) ).

kaikreuzer · 2016-08-03T07:37:23Z

I'd assumed that these extensions were also going to be available in other rules as well

Maybe only in some changed form and definitely without joda (hoping we then have Java8 instead) - so this refactoring might be done later.

no-one really has 'time'

How true :-/ Anyhow, I'll look into it now!

kaikreuzer · 2016-08-03T11:35:40Z

@cdjackson I have created #1962.
Might I ask you to review it?

cdjackson · 2016-08-03T13:23:06Z

Sure - will be done early evening...

Initial commit of persistence extensions

12c7c31

Signed-off-by: Chris Jackson <[email protected]>

kaikreuzer reviewed Jul 19, 2016
View reviewed changes

Updated API

b532fa5

Signed-off-by: Chris Jackson <[email protected]>

kaikreuzer reviewed Jul 22, 2016
View reviewed changes

maggu2810 reviewed Jul 22, 2016
View reviewed changes

Updated following review and added REST resource updates

dfbbf7a

Signed-off-by: Chris Jackson <[email protected]>

cdjackson force-pushed the persistence-extensions branch 4 times, most recently from 8d86c5a to b0791d3 Compare July 22, 2016 20:04

cdjackson force-pushed the persistence-extensions branch 2 times, most recently from 22e805f to 76e2938 Compare July 23, 2016 13:14

Add H2SQL persistence bundle

bcaa729

Signed-off-by: Chris Jackson <[email protected]>

cdjackson force-pushed the persistence-extensions branch from 76e2938 to bcaa729 Compare July 23, 2016 15:55

maggu2810 reviewed Jul 23, 2016
View reviewed changes

kaikreuzer reviewed Aug 2, 2016
View reviewed changes

Updated following comments.

6610b71

Signed-off-by: Chris Jackson <[email protected]>

Update tests

cbcd900

Signed-off-by: Chris Jackson <[email protected]>

kaikreuzer merged commit f7ec19f into eclipse-archived:master Aug 2, 2016

kaikreuzer mentioned this pull request Aug 3, 2016

my.openHAB Connection Service failed to build openhab/openhab-addons#1173

Closed

cdjackson deleted the persistence-extensions branch August 3, 2016 13:23

kaikreuzer mentioned this pull request Oct 15, 2016

Add a persistence REST interface #631

Closed

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

wborn mentioned this pull request Dec 17, 2017

Historic state updates from bindings #4765

Closed

wborn mentioned this pull request May 31, 2019

Historic state updates from bindings openhab/openhab-core#844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit of persistence extensions #1872

Initial commit of persistence extensions #1872

cdjackson commented Jul 17, 2016

kaikreuzer Jul 19, 2016

cdjackson Jul 19, 2016

kaikreuzer Jul 19, 2016

cdjackson Jul 19, 2016

cdjackson commented Jul 19, 2016

kaikreuzer commented Jul 19, 2016

kaikreuzer Jul 19, 2016

cdjackson commented Jul 19, 2016

cdjackson commented Jul 21, 2016

kaikreuzer Jul 22, 2016

cdjackson Jul 22, 2016

kaikreuzer commented Jul 22, 2016

maggu2810 Jul 22, 2016

maggu2810 Jul 22, 2016

cdjackson Jul 22, 2016

cdjackson commented Jul 22, 2016

cdjackson commented Jul 23, 2016

maggu2810 commented Jul 23, 2016

maggu2810 Jul 23, 2016

cdjackson commented Aug 2, 2016

kaikreuzer Aug 2, 2016

cdjackson commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

cdjackson commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

cdjackson commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

cdjackson commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

cdjackson commented Aug 3, 2016

Initial commit of persistence extensions #1872

Initial commit of persistence extensions #1872

Conversation

cdjackson commented Jul 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdjackson commented Jul 19, 2016

kaikreuzer commented Jul 19, 2016

Choose a reason for hiding this comment

cdjackson commented Jul 19, 2016

cdjackson commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdjackson commented Jul 22, 2016

cdjackson commented Jul 23, 2016

maggu2810 commented Jul 23, 2016

Choose a reason for hiding this comment

cdjackson commented Aug 2, 2016

Choose a reason for hiding this comment

cdjackson commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

cdjackson commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

kaikreuzer commented Aug 2, 2016

cdjackson commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

cdjackson commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

kaikreuzer commented Aug 3, 2016

cdjackson commented Aug 3, 2016