openhab / openhab1-addons

Add-ons for openHAB 1.x
Eclipse Public License 2.0
3.43k stars 1.69k forks source link

Persistence API breaks compatibility to openHAB 2 #2661

Closed kaikreuzer closed 8 years ago

kaikreuzer commented 9 years ago

In 1.6.1 a new service interface has been introduced: https://github.com/openhab/openhab/blob/master/bundles/core/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistentStateRestorer.java

This does was not synced to ESH and thus it potentially breaks the compatibility of persistence services with openHAB 2. I do not know the reasons for this change in the core runtime, but it might needs to be revisited as the parameter name "modelName" hints at the fact that there is a logical reference to DSL files here, which must not be the case from a core bundle (and hence breaks the architecture).

teichsta commented 9 years ago

@gerrieg since the changes were initiated by you at that time, could you elaborate a bit on this? Thanks, Thomas E.-E.

gerrieg commented 9 years ago

There was a timing problem with restoreOnStartup, see https://github.com/openhab/openhab/pull/1757

kaikreuzer commented 9 years ago

Imho, #1757 should not have been merged, it introduces a bi-directional dependency between the PersistenceManager and the PersistenceServices. As far as I understand the problem is only for services that require a configuration. These should imho use configuration-policy="require" and handle the configuration in the activate() method and not implement ManagedService at all.

Please address this issue as it is marked critical (see also https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/openhab2/XX4f40bJ5vw/vRc3HcIbJKcJ)

lewie commented 9 years ago

Hi @kaikreuzer so I've implemented it in this Persistence Service https://github.com/openhab/openhab/issues/2804, incl. configuration-policy = "require", unfortunately, the activate () function seems not to be called in openHab2.

Only for information. Rgds Helmut

kaikreuzer commented 9 years ago

@lewie Do you have the code available, so that I can help analyzing why it is not activated on O2?

lewie commented 9 years ago

Hi @kaikreuzer

PostgreSQL-new-binding code

Functionality is like as in MySql Service but without ManagedService. Its working well in OH1.

Thank You Helmut

kaikreuzer commented 9 years ago

@lewie, this code still refers to the PersistentStateRestorer, see https://github.com/openhab/openhab/compare/master...lewie:PostgreSQL-new-binding?diff=unified&expand=1&name=PostgreSQL-new-binding#diff-3e7cf4102c7c8349b09cd63db28556f9R54.

As mentioned in https://github.com/openhab/openhab/issues/2661#issuecomment-123662806, #1757, which introduced this class, should never have been merged and imho needs to be reverted.

TeraHz commented 9 years ago

FWIW I just removed the PersistentStateRestorer references from the MySQL bundle and it started working in OH2 (also fixed https://github.com/openhab/openhab2/issues/321)

https://github.com/TeraHz/openhab/commit/72d292832f78271e9dd47307d9024a870a596435

I assume this provided some functionallity so the proper fix is likely more complicated, but if you think this will be useful for others for testing, I can issue a pull request.

kaikreuzer commented 9 years ago

and it started working in OH2

That's good news!

I assume this provided some functionallity so the proper fix is likely more complicated

See my comment from https://github.com/openhab/openhab/issues/2661#issuecomment-123662806:

As far as I understand the problem is only for services that require a configuration. These should imho use configuration-policy="require" and handle the configuration in the activate() method and not implement ManagedService at all.

TeraHz commented 9 years ago

That's strange because the service definitely requires a configuration to connect and that still works.

From what I can tell, the configuration is read by updated(). I'll check tonight but it is probably called on startup as well that's why it is still working?

kaikreuzer commented 9 years ago

Are you also specifically testing the restoreOnStartup option? This was the reason for introducing the PersistentStateRestorer (see https://github.com/openhab/openhab/issues/2661#issuecomment-106511422)

TeraHz commented 9 years ago

I don't think I am, will do tonight.

I left the call to persistentStateRestorer.initializeItems(getName()); and it isn't failing (or if it is it happens silently). I'll double check later today.

TeraHz commented 9 years ago

OK, just checked and I do have restoreOnStartup for all my items and it works just fine. If I remove it I can confirm that none of the items are populated and they get populated little by little. With the option in they are restored on startup.

Not sure how it is working but the only other persistence addon that I have is the logging.

Do you want me to test anything else?

kaikreuzer commented 9 years ago

Thanks @TeraHz!

@teichsta Do you track that this is solved in general and not just for MySQL? Although I marked that as critical, there hasn't been much activity here yet...

teichsta commented 9 years ago

i'll take care of this in the next week …

grzech1983 commented 9 years ago

Hello, is there anything new with support for MySQL persistance in OH2?

teichsta commented 9 years ago

hmm … thanks for the ping! I've started some weeks ago but didn't finalised the change!

grzech1983 commented 9 years ago

Great. Thanks for update.

teichsta commented 9 years ago

see #3125 - this should re-establish compatibility with oh2 and the compaq-layer. Appreciate your feedback @grzech1983 …

grzech1983 commented 9 years ago

Does this change will be added in cloudbees builds tomorrow? I could test it if I had compliled MySQL provider

teichsta commented 9 years ago

yep!

grzech1983 commented 9 years ago

Great. I will let you know tomorrow

grzech1983 commented 9 years ago

Does this change will be reported in changelog or I just should download latest addons package from 1.8.x version and use MySQL persistance binding?

teichsta commented 9 years ago

which changelog do you mean? just download the latest build from https://openhab.ci.cloudbees.com/job/openHAB/

grzech1983 commented 9 years ago

I was referring to this change log:

https://openhab.ci.cloudbees.com/job/openHAB/changes

Yesterday I've downloaded latest compiled addons and in mysql TRACE log I can see only;

2015-09-05 10:09:42.240 DEBUG o.o.p.m.i.MysqlPersistenceServiceActivator[:32]- mySQL persistence bundle has been started. 2015-09-05 10:10:50.408 DEBUG o.o.p.m.i.MysqlPersistenceService[:500]- mySQL configuration starting 2015-09-05 10:10:50.422 DEBUG o.o.p.m.i.MysqlPersistenceService[:427]- mySQL: Attempting to connect to database jdbc:mysql://127.0.0.1/openhab 2015-09-05 10:10:53.722 DEBUG o.o.p.m.i.MysqlPersistenceService[:430]- mySQL: Connected to database jdbc:mysql://127.0.0.1/openhab 2015-09-05 10:10:53.805 DEBUG o.o.p.m.i.MysqlPersistenceService[:561]- mySQL configuration complete.

I have configuration file in conf/persistance/mysql.persist and conf/services/mysql.cfg.

I've also set:

org.eclipse.smarthome.persistence:default=mysql

in runtime/etc/services.cfg

When I'm logging in as MySQL admin I don't see any client connections for user openhab. Any suggestions?

teichsta commented 9 years ago

The log seem to look promising (assuming your MySQL server runs on 127.0.0.1). Does the persistence service work generally? So is any data stored to MySQL? If not can anybody of @lewie @TeraHz or @gerrieg help?

lewie commented 9 years ago

@teichsta, @grzech1983, actual Version of mysql Bundle seems not to work in OH1. Its a problem of getting configuration. I test straight!

Rgds Helmut

teichsta commented 9 years ago

hmm … would be great if you could investigate further. For me the bundle works perfectly fine … (if a configuration has been added to openhab.cfg)

lewie commented 9 years ago

@teichsta, @grzech1983, HA, YES is does work perfectly!!

Actually, in my testing OH1 path I had JDBC driver Bundles for mariaDB and mysql 5.1.36 ( for testing my new DBMS Bundle). They are jostling in front to original jdbc driver (mysql-connector-java-5.1.26-bin.jar).

Only for information: Tested: Mysql-bundle runs perfectly with mysql-connector-java-5.1.26-bin.jar or newer mysql-connector-java-5.1.36.jar driver embedded. But if I additionally copy same Driver to the path, the Bundle does not start correct.

Rgds Helmut

kaikreuzer commented 9 years ago

Can anybody here confirm that we can remove the 3 entries at https://github.com/openhab/openhab2/blob/master/docs/sources/addons.md#currently-incompatible-1x-add-ons and have them listed as compatible with openHAB 2?

grzech1983 commented 9 years ago

Hello,

I've downloaded again addons package from cloudbees and tried again MySQL addon and now I see in mysql log that entries are being added to MySQL database. I was using JPA addon for few last days and it was working fine (https://community.openhab.org/t/openhab2-jpa-binding-mysql-db/2375). When I switched to MySQL addon charts stopped working. Any idea where I should look for resolving this issue?

Thanks

kaikreuzer commented 9 years ago

The fix through #3125 is not correct. It makes persistence services not start up anymore if there is no configuration available. I have fixed it for rrd4j in #3168. Please fix the other services accordingly and test if they work before closing this issue - thanks!

staehler commented 9 years ago

I there any progress? As far as I undersand this, every persistence service has to be adopted, correct? Not sure if my above mentioned influxdb problem relates to this, but I'm keen on a fix, as I can not upgrade to further snapshots, since the issue #3173.

teichsta commented 9 years ago

please find #3179 which incorporates the changes @kaikreuzer suggested. Please comment if that fix works for your @staehler.

staehler commented 9 years ago

@kaikreuzer @teichsta This fix isn't merged till now, correct. I tried Snapshot 1022 and the influxDB error during startup remains unchanged. I switched back to my running version 1001. I'll give feedback, when you think the bug should be fixed.

teichsta commented 9 years ago

yes, correct, this PR has not yet been merged into master since it would be great to have beforehand. Could possibly build openHAB from the PR yourself? Thanks for your help, Thomas E.-E.

staehler commented 9 years ago

@teichsta Unfortunately I've no idea, how to set up the correct environment and compiler to do that. I'm just a user, no developer ... sorry!

teichsta commented 9 years ago

ok, i've enabled the auto-build again. So you should the binaries for the according PR here: https://openhab-ci.innoq.io//jenkins/job/openhab-pr-builder/9/ could you please try and see if everything works now …

teichsta commented 9 years ago

ok, i've merged #3179. Please confirm (with the upcoming nightly build) that every works fine again.

kaikreuzer commented 9 years ago

Any news here? I would like to update https://github.com/openhab/openhab2/blob/master/docs/sources/addons.md#currently-incompatible-1x-add-ons some when...

teichsta commented 9 years ago

according to #3173 this one can be closed now …

kaikreuzer commented 9 years ago

3173 only talks about InfluxDB, but not about MySQL and MongoDB. Can you confirm that they work as well?

teichsta commented 9 years ago

@lewie since you seem to be one of your MySQL PowerUsers … can you?! :-)

lewie commented 9 years ago

@kaikreuzer, @teichsta, yes, will test this, if possible tomorrow ...

Wouldn't it make sense to push forward integration of Generic JDBC Persistence (https://github.com/openhab/openhab/pull/3248) instead? It is precisely this double work (databases with JDBC skills) is supposed to prevent the generic bundle ... ;-)

Rgds Helmut

teichsta commented 9 years ago

yes, will test this, if possible tomorrow ...

any update, @lewie? Would be great to see this issue fixed first before working on new integrations. Thanks, Thomas E.-E.

lewie commented 8 years ago

Hi @teichsta,

the current MySQL Bundle Source works as expected in OH 1 and OH 2.

The behavior is identical, except for the two item types external call and location, of course, are in OpenHab 2 no longer or not yet available.

MongoDB, I can't test yet, will try again later. Studying the source and bundle configuration it is all right:

Rgds Helmut

lewie commented 8 years ago

@teichsta, I have tested MongoDB in OH1 and OH2 now, It is compatible with OH2. If OH2 has no config-file for mongodb we get warnings for missing config Parameters, no loop.

Rgds Helmut

kaikreuzer commented 8 years ago

Thanks a lot for testing @lewie! We can therefore remove the entries at https://github.com/openhab/openhab2/blob/master/docs/sources/addons.md#currently-incompatible-1x-add-ons - wonderful :-)