allow non-model based persistence configuration #2094

maggu2810 · 2016-09-01T15:04:15Z

Currently a persistence service could be only configured using the DSL / models files (.persist).
This PR moves the persistence manager logic (the non-model based one) to persistence core and allows a programmatically configuration of persistence services, too. So we could add a REST interface some time.

It is far from complete, but it should at least work.
I used some "stupid" names, starting with "Simple". This has been done to prevent collisions between the names used in the model at development but will be changed later.

Can you have a look at?
@cdjackson As you seems to be a professional developer of the ESH persistence interfaces, could you have a look at, too?
@kaikreuzer The most files are touched has been written by you. Your comments are very welcome.

There is a service for org.eclipse.smarthome.core.persistence.PersistenceManager now.
This service provides functions to:

add persistence service configuration (will also trigger 'start event handling' for this persistence service)
remove persistence service configuration (will also trigger 'stop event handling' for this persistence service)
start event handling for persistence service
stop event handling for persistence service

kaikreuzer · 2016-09-01T15:55:51Z

bundles/core/org.eclipse.smarthome.core.persistence/META-INF/MANIFEST.MF

 org.eclipse.smarthome.core.items,
 org.eclipse.smarthome.core.library.types,
 org.eclipse.smarthome.core.persistence,
 org.eclipse.smarthome.core.types,
+ org.quartz,


This is a problem. You might remember that we tried to get rid off Quartz in the core bundles (see #658), since it is not compatible with compact2 profile and also is pretty fat. So when overhauling the persistence APIs, I would definitely refrain from using this library.
I would hope that you can use @kgoderis' new scheduler implementation instead.

I know 😉
I used quartz just temporary to not change to much code.
I will use the new scheduler implementation (the automation should move its timer trigger, too) after the concept of the split is okay.

kaikreuzer · 2016-09-01T16:05:33Z

Thanks @maggu2810, making the persistence stuff independent from Xtext+EMF is definitely an important step (which was far too low on my personal todo list ;-)).

So apart from the Quartz-refactoring, I would like to start the discussion what other refactorings we might want to do in order to adapt the persistence API / configuration possibilities to the needs.
The current setup with the strategies etc. is something I came up with a very long time ago and it hasn't evolved since then. Now is a chance to do changes and enhancements to it, so use the chance!

maggu2810 · 2016-09-01T17:37:22Z

So apart from the Quartz-refactoring

Done, no Quartz for persistence anymore 😉

kaikreuzer · 2016-09-02T08:49:44Z

Done, no Quartz for persistence anymore 😉

Cool. Did you also test that it is working? I never tried the new scheduler with cron stuff myself yet...

So what remains is my question in #2094 (comment).
I'd also like to invite other to think about any requirements and wishes here - @cdjackson, @kgoderis, @SJKA and others - please join in!

maggu2810 · 2016-09-02T09:07:29Z

Cool. Did you also test that it is working?

I tried this strategy everySecond : "* * * * * ?" and this one is working using the ESH scheduler (ExpressionThreadPoolExecutor).

kgoderis · 2016-09-02T16:40:41Z

@maggu2810 @kaikreuzer Damn... 8hours to late ;-) I have this piece of code in the pipe : kgoderis@826fc78

I had already the de-quartzifying on my to do list - the url here above could serve as an example.

kgoderis · 2016-09-02T16:43:29Z

@maggu2810 btw, forget about cron, use RFC 5545 ;-)

maggu2810 · 2016-09-02T17:13:35Z

Damn... 8hours to late ;-) I have this piece of code in the pipe : kgoderis/smarthome@826fc78

@kgoderis I removed Quartz from the persistence service only.
So, all the remaining usages could be changed by you 😉

kgoderis · 2016-09-02T17:18:05Z

So, all the remaining usages could be changed by you 😉

Yeah Yeah - the todo list is growing every day ....

kaikreuzer · 2016-09-15T09:05:04Z

I'd be fine to go ahead with this PR. @maggu2810 Do you still consider it WIP? Or shall we merge it as a working replacement for the current code and discuss further evolution in future?

maggu2810 · 2016-09-15T09:18:15Z

I think I need to revert the change to use the ESH scheduler instead of Quartz because cron does not work with the current ESH scheduler implementation as expected (see #2105)

kaikreuzer · 2016-09-15T09:26:23Z

I didn't follow #2105 in detail, but is this something that cannot easily be fixed by @kgoderis and we simply wait for that?

maggu2810 · 2016-09-15T11:56:42Z

but is this something that cannot easily be fixed by @kgoderis

I don't know if it is such easily. But I assume @kgoderis knows.

and we simply wait for that?

That is the reason why I kept the [WIP] in front of the title.

Or shall we merge it as a working replacement for the current code and discuss further evolution in future?

I assume it is simple to merge the split of model.persistence now (without the Quartz scheduler removal) and merge the Quartz scheduler removal (the commits could be simple split out of this PR) after the ESH scheduler has been fixed.

kaikreuzer · 2016-09-15T12:00:31Z

I assume it is simple to merge the split of model.persistence now (without the Quartz scheduler removal)

Not really, because this step moves the usage of Quartz from model to core. As the model bundles are optional, not everyone has this dependency, but if we move it to core, it will force people to add (the fat) Quartz to their stack. I'd therefore rather would want to wait for a solution of #2105.

maggu2810 · 2016-09-15T12:06:01Z

this step moves the usage of Quartz from model to core

Okay 😉, missed that point

kgoderis · 2016-09-16T11:44:54Z

I didn't follow #2105 in detail, but is this something that cannot easily be fixed by @kgoderis and we simply wait for that?

Yoda says: Easy it is not

I have not revisted #2105 since the discussion we had on the subject, as we need to decide on a strategy (see that thread) before moving forward. @kaikreuzer maybe you can voice your thoughts on the matter if you happen to find some time?

kaikreuzer · 2016-09-16T13:46:06Z

if you happen to find some time

I won't for the next days, but WHEN I find time, I will try to thing about it and share my thoughts...

kgoderis · 2016-10-08T06:26:44Z

@maggu2810 Pls see PR #2270

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

maggu2810 · 2016-10-25T20:59:32Z

#2270 has been merged, so #2105 should be fixed

kaikreuzer · 2016-11-02T14:37:09Z

...ce/src/main/java/org/eclipse/smarthome/core/persistence/internal/PersistenceManagerImpl.java

+ return;
+ }
+ for (final Runnable job : persistenceJobs.get(persistModelName)) {
+ boolean success = scheduler.remove(job);


I just started testing this PR, but in this line success is always false and I get:

2016-11-02 15:25:55.180 [WARN ] [o.e.s.c.p.PersistenceManager :317 ] - Failed to delete cron job for dbId 'rrd4j

in my log (when updating the file rrd4j.persist).
This is probably an issue in the ESH scheduler implementation, but I'd like to know if this has worked for you? In my tests, the scheduler keeps creating new jobs and never removes them again...

I have created https://github.com/eclipse/smarthome/pull/2391/files#diff-6ab8869df39e4b2f961a094a0437b037R34 for further investigation and fixing.

I assume this should be fixed by #2435, now. Correct?

kaikreuzer · 2016-11-02T14:51:40Z

...ce/src/main/java/org/eclipse/smarthome/core/persistence/internal/PersistenceManagerImpl.java

+ private final Map<String, Set<Runnable>> persistenceJobs = new HashMap<>();
+
+ public PersistenceManagerImpl() {
+ scheduler = ExpressionThreadPoolManager.getExpressionScheduledPool("persist");


wouldn't it be better to do this in activate() and de-reference and dispose it again in deactivate()?

Yes, I think so.

Will change it.

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

maggu2810 · 2016-12-09T16:33:14Z

Hm, what are we missing here?

kaikreuzer · 2016-12-09T16:43:43Z

Trust in the new CRON scheduler implementation...
Sorry, I didn't yet do any further tests, but my feeling is that it would be good to test it on a real system running for at least 24 hours.

maggu2810 · 2016-12-09T16:54:25Z

The PR to "allow non-model based persistence configuration" also use the ESH scheduler instead of the Quartz one.
For sure we should not merge it without some tests.
Have you some ideas how such a test case should look like?
I could keep a system running for some days that use different CRON expression to store a value.
As only the CRON scheduler needs to be tested, there is no "real" hardware or state changes necessary. Correct?
We need only to ensure that the CRON expression are still triggered after some days. Correct?

kaikreuzer · 2016-12-09T18:31:51Z

Correct. What should be checked is whether there is any change in average system load and in the memory footprint. My plan was to test it with the openHAB demo setup, which has a couple of cron persistence configurations. Didn't yet find the time, tough.

kaikreuzer · 2017-01-04T13:02:13Z

Ok, I finally did some testing. I couldn't make out any unusual CPU or memory consumption, even under high (persistence) load, so it lgtm.

The problem is that the initial job does not seem to be correctly scheduled. After a startup, no items are persisted. Once I touch the rrd4j.perist file, I see a

13:56:39.800 [WARN ] [.core.persistence.PersistenceManager] - Failed to delete cron job for dbId 'rrd4j'

in my log and afterwards it starts working.

Not sure whether this is an issue of this PR or of the new scheduler service. Will need to investigate, before this can be merged.

kaikreuzer · 2017-01-04T15:25:52Z

Ok, I tracked it down to a bug in the scheduler: #2746
As long as this isn't fixed, we cannot merge this PR here.

maggu2810 · 2017-01-04T15:31:10Z

Thanks for tracking that down @kaikreuzer

kaikreuzer · 2017-01-13T12:50:46Z

#2746 is fixed, so we are good to merge!

* allow non-model based persistence configuration Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

kaikreuzer reviewed Sep 1, 2016
View reviewed changes

kaikreuzer added Core Persistence labels Sep 15, 2016

kgoderis mentioned this pull request Oct 6, 2016

Scheduler : fix #2105 #2270

Merged

maggu2810 added 4 commits October 9, 2016 21:39

allow non-model based persistence configuration

a70fa80

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

persistence-model: use esh scheduler instead quartz

9c02f66

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

persistence-model: remove unused quartz from persistence model

e50e69a

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

persistence-model: fix docu

7f8c9dc

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

maggu2810 changed the title ~~[WIP] allow non-model based persistence configuration~~ allow non-model based persistence configuration Oct 25, 2016

kaikreuzer reviewed Nov 2, 2016

View reviewed changes

kaikreuzer mentioned this pull request Nov 3, 2016

Fix bugs in ESH scheduler and add more unit tests #2394

Closed

kgoderis mentioned this pull request Nov 22, 2016

Cron Expression : Fix #2394 #2435

Merged

move scheduler handling to (de)activate methods

d5aad18

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

kaikreuzer mentioned this pull request Jan 4, 2017

CronExpression fails on execution on every minute #2746

Closed

kaikreuzer merged commit 858698d into eclipse-archived:master Jan 13, 2017

maggu2810 deleted the persistence-model branch January 13, 2017 13:37

chaton78 pushed a commit to chaton78/smarthome that referenced this pull request May 7, 2017

allow non-model based persistence configuration (eclipse-archived#2094)

adff1ba

* allow non-model based persistence configuration Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

maggu2810 mentioned this pull request Apr 5, 2019

Refactored persistence api to org.openhab and removed compatibility layer for persistence services openhab/openhab-core#680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow non-model based persistence configuration #2094

allow non-model based persistence configuration #2094

maggu2810 commented Sep 1, 2016

kaikreuzer Sep 1, 2016

maggu2810 Sep 1, 2016

kaikreuzer commented Sep 1, 2016

maggu2810 commented Sep 1, 2016 •

edited

Loading

kaikreuzer commented Sep 2, 2016

maggu2810 commented Sep 2, 2016

kgoderis commented Sep 2, 2016

kgoderis commented Sep 2, 2016

maggu2810 commented Sep 2, 2016

kgoderis commented Sep 2, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kgoderis commented Sep 16, 2016

kaikreuzer commented Sep 16, 2016

kgoderis commented Oct 8, 2016

maggu2810 commented Oct 25, 2016

kaikreuzer Nov 2, 2016 •

edited

Loading

kaikreuzer Nov 2, 2016

maggu2810 Nov 24, 2016

kaikreuzer Nov 2, 2016

maggu2810 Nov 24, 2016

maggu2810 commented Dec 9, 2016

kaikreuzer commented Dec 9, 2016

maggu2810 commented Dec 9, 2016 •

edited

Loading

kaikreuzer commented Dec 9, 2016

kaikreuzer commented Jan 4, 2017

kaikreuzer commented Jan 4, 2017

maggu2810 commented Jan 4, 2017

kaikreuzer commented Jan 13, 2017

allow non-model based persistence configuration #2094

allow non-model based persistence configuration #2094

Conversation

maggu2810 commented Sep 1, 2016

kaikreuzer Sep 1, 2016

Choose a reason for hiding this comment

maggu2810 Sep 1, 2016

Choose a reason for hiding this comment

kaikreuzer commented Sep 1, 2016

maggu2810 commented Sep 1, 2016 • edited Loading

kaikreuzer commented Sep 2, 2016

maggu2810 commented Sep 2, 2016

kgoderis commented Sep 2, 2016

kgoderis commented Sep 2, 2016

maggu2810 commented Sep 2, 2016

kgoderis commented Sep 2, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kaikreuzer commented Sep 15, 2016

maggu2810 commented Sep 15, 2016

kgoderis commented Sep 16, 2016

kaikreuzer commented Sep 16, 2016

kgoderis commented Oct 8, 2016

maggu2810 commented Oct 25, 2016

kaikreuzer Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

kaikreuzer Nov 2, 2016

Choose a reason for hiding this comment

maggu2810 Nov 24, 2016

Choose a reason for hiding this comment

kaikreuzer Nov 2, 2016

Choose a reason for hiding this comment

maggu2810 Nov 24, 2016

Choose a reason for hiding this comment

maggu2810 commented Dec 9, 2016

kaikreuzer commented Dec 9, 2016

maggu2810 commented Dec 9, 2016 • edited Loading

kaikreuzer commented Dec 9, 2016

kaikreuzer commented Jan 4, 2017

kaikreuzer commented Jan 4, 2017

maggu2810 commented Jan 4, 2017

kaikreuzer commented Jan 13, 2017

maggu2810 commented Sep 1, 2016 •

edited

Loading

kaikreuzer Nov 2, 2016 •

edited

Loading

maggu2810 commented Dec 9, 2016 •

edited

Loading