Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrated persistence services from openhab1-addons #5275

Merged
merged 6 commits into from
Mar 24, 2020

Conversation

kaikreuzer
Copy link
Member

@kaikreuzer kaikreuzer commented Mar 27, 2019

Depends on openhab/openhab-core#1387

Signed-off-by: Kai Kreuzer kai@openhab.org

@davidgraeff davidgraeff added the enhancement An enhancement or new feature for an existing add-on label Mar 30, 2019
kaikreuzer referenced this pull request Mar 31, 2019
This currently crashes our documentation deployment as it clashes with the official MapDB persistence service from openhab1-addons.
https://github.com/openhab/openhab2-addons/pull/5275 will merge the two into a single bundle and provide the official README with it.
@wborn wborn added new persistence and removed enhancement An enhancement or new feature for an existing add-on labels Mar 31, 2019
davidgraeff referenced this pull request Apr 1, 2019
This currently crashes our documentation deployment as it clashes with the official MapDB persistence service from openhab1-addons.
https://github.com/openhab/openhab2-addons/pull/5275 will merge the two into a single bundle and provide the official README with it.
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-develop-a-new-persistence-addon/73105/7

@ssalonen
Copy link
Contributor

PR for DynamoDB: kaikreuzer#4

@kaikreuzer
Copy link
Member Author

@davidgraeff Question to you as the MQTT expert: The MQTT persistence is linked to the MQTT 1.x binding. Is there already any similar functionality in 2.x? Are people using it at all / is it still useful?
Will we need to adapt the persistence service to the new MQTT support?

@davidgraeff
Copy link
Member

Is there already any similar functionality in 2.x?

No not yet.

Are people using it at all / is it still useful?

People are not using it currently. There was not a single complain with the big OH 2.4 MQTT introduction that the mqtt1 persistence broke for anyone.

Will we need to adapt the persistence service to the new MQTT support?

I'm not entirely sure what this persistence is supposed to do to be honest. A 5 lines rule can already push every item change to specific MQTT topics and I want to add OH<-->OH eventbus synchronisation via MQTT as well. I really don't see a reason for this extension?!

@kaikreuzer
Copy link
Member Author

I agree that this isn't a very good match for a persistence service - so let's drop it here and we can still re-introduce it, if somebody comes up with a good use case for it.

@kaikreuzer
Copy link
Member Author

@derekadams: Trying to migrate the Sitewhere persistence service, it deemed to me that this code is quite old and might not be working with the new Sitewhere 2.0 service anymore - the used MQTT communication rather seems to be replaced by a REST API, and the referenced tutorial at https://sitewhere.io/integration/openhab.html meanwhile results in a 404. Instead of putting much effort into it, I'll rather remove it here. If you believe that it is worthwhile to update it and keep it available for users, please let us know and help on getting it up-to-date again!

@kaikreuzer
Copy link
Member Author

Putting this on hold for the moment, see openhab/openhab-core#680 (comment).

@kaikreuzer kaikreuzer added the awaiting other PR Depends on another PR label Apr 28, 2019
@derekadams
Copy link

@kaikreuzer We are planning to do a new persistence service for the SiteWhere 2.0 architecture since some of the interaction patterns have changed. Thanks for taking a look at the existing code. You're correct that we leverage the updated REST APIs to do much of the work now rather than sending the payloads via MQTT. We may support both options, but will know more once we dive back into the persistence service code. Thanks!

@kaikreuzer
Copy link
Member Author

Thanks @derekadams, I'll let you know once this is merged (after the 2.5 release) and I'll remove the old Sitewhere service, so that we can add the new one then!

@JueBag
Copy link

JueBag commented May 6, 2019

I observed an issue concerning rrd4j ( filed here ).
.rrd files are created when performing a REST call although the items are not persisted.
Not sure if that is an issue or a feature.

Pshatsillo referenced this pull request in Pshatsillo/openhab-addons Jun 19, 2019
This currently crashes our documentation deployment as it clashes with the official MapDB persistence service from openhab1-addons.
https://github.com/openhab/openhab2-addons/pull/5275 will merge the two into a single bundle and provide the official README with it.

Signed-off-by: Pshatsillo <pshatsillo@gmail.com>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/poll-which-oh1-x-addons-do-you-use/79121/1

ne0h referenced this pull request in ne0h/openhab-addons Sep 15, 2019
This currently crashes our documentation deployment as it clashes with the official MapDB persistence service from openhab1-addons.
https://github.com/openhab/openhab2-addons/pull/5275 will merge the two into a single bundle and provide the official README with it.

Signed-off-by: Maximilian Hess <mail@ne0h.de>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/problem-with-new-addon-and-dependencies-packaging/85658/2

@kaikreuzer
Copy link
Member Author

@ssalonen, @lewie and others: Now that those services are migrated and available in the 3.0-SNAPSHOT distro, we absolutely count on your to check whether your persistence services are functional - see my comment above (#5275 (comment)). Would be great, if you could go for this asap to make 3.0 a true replacement for 2.x!

@lewie
Copy link

lewie commented Apr 5, 2020

@kaikreuzer,
very cool, an important step!

My detailed test scripts are written in Javascript and as far as I know there is currently no rule binding that can handle them for openHAB 3. Or is there? Now I tested the general installation and basic things like reading, writing and historical functions. The only thing is the CallType, which did not work, but it is outdated anyway?!

I have tested now, all JDBC Binding variants.

Tested persistence JDBC add-ons:

Title Sign Driver/Installation read/write
DynamoDB dynamodb untested untested
InfluxSB influxdb untested untested
JDBC H2 jdbc-h2 ok ok
JDBC HSQLDB jdbc-hsqldb ok ok
JDBC MariaDB jdbc-mariadb ok ok
JDBC MySQL jdbc-mysql ok ok
JDBC PostgreSQL jdbc-postgresql ok ok
JDBC SQLite jdbc-sqlite ok ok
JPA jpa untested untested
MapDB mapdb untested untested
MongoDB mongodb untested untested
MySQL mysql untested untested

So, all tests I have done, turned out as expected, as in openHAB 2, in fact everything worked! 👍 💯 points!
Thank you very much, Kai for porting this!

One more remark. The second mysql binding (also JDBC) is redundant. At that time I built the JDBC binding for derby, h2, hsqldb, mariadb, mysql, postgres, sqlite, this was based on the original mysql binding. It was done just to reduce the redundancy of the same code for all JDBC based Drivers for databases. Even the database structure is fully compatible. Since the second obsolete mysql binding has no advantages, I suggest to finally let it die in openHAB 3 and recommend the JDBC-MySQL binding to users. What do you think about this saving of redundancy to maintain the code?

@kaikreuzer
Copy link
Member Author

Great, thank you very much for testing, the result is much better than I had hoped!

written in Javascript and as far as I know there is currently no rule binding that can handle them for openHAB 3

Good question, I am actually not fully up to date on the status. But I am just working on removing the old rule engine and making the NGRE be installed by default and I think once we are there, it should not take long to have JS scripting back as well.

The only thing is the CallType, which did not work, but it is outdated anyway?!

It is correct that the CallType does not exist anymore. But the CallItem is still there, it now only uses the StringListType internally. So it would be important to test this item type as well (and adapt the code to handle the StringListType correctly, which I indeed haven't done).

What do you think about this saving of redundancy to maintain the code?

Yes, if the JDBC-MySQL service works as well as the other, I absolutely agree that we should remove the specific one! I'll create a PR immediately...

@kaikreuzer
Copy link
Member Author

... done: #7306

@ssalonen
Copy link
Contributor

ssalonen commented Apr 5, 2020

It is correct that the CallType does not exist anymore. But the CallItem is still there, it now only uses the StringListType internally. So it would be important to test this item type as well (and adapt the code to handle the StringListType correctly, which I indeed haven't done).

The dynamodb integration tests failed with the CallItems due to this due to sorting differences, which is again due to changed serialization format (dynamodb persistence uses state.tostring). But this made me think..

Do we need to handle this in persistence bindings ? Otherwise old data is not readable.

Are there other breaking changes in State.toString with other item types?

@kaikreuzer
Copy link
Member Author

Do we need to handle this in persistence bindings ? Otherwise old data is not readable.

Pfew, good question. I actually doubt that anybody ever wants to read old CallItem states back, so I would say, we can document this as a breaking change and be fine with that. It is really a very small edge case and imho not worth to carry migration code in the add-on with us.

Are there other breaking changes in State.toString with other item types?

No, not that I am aware of any.

@lewie
Copy link

lewie commented Apr 7, 2020

@kaikreuzer,
I am really happy, CallType works with string as value without errors! The data is stored as chars in the databases anyway. 👍

Great, thank you very much for testing, the result is much better than I had hoped!

...not just you!

...it should not take long to have JS scripting back as well.

I can hardly wait!

Yes, if the JDBC-MySQL service works as well as the other, I absolutely agree that we should remove the specific one! I'll create a PR immediately...

Thanks, I think it's a good time for the deletion! And yes, there is also a point in the documentation about this: migration-from-mysql-to-jdbc-persistence-services

Fragmentarily, I have followed the discussions the last Month. But now first time I have dealt with the current state of openHAB 3, great work, compliments to you all!
I am extremely excited about the new parts like the speed of the surface, that is contemporary!

@wborn wborn added this to the 3.0 milestone Apr 25, 2020
@ssalonen
Copy link
Contributor

ssalonen commented May 9, 2020

@kaikreuzer @lewie how to configure the persistence addon in 3.0? Finally got the IDE resolved and the demo app launched but I do not see any obvious way to configure the addon. There is no runtime/conf/services folder anymore.

Trying to test out the dynamodb with the full setup.

@kaikreuzer
Copy link
Member Author

There is no runtime/conf/services folder anymore.

There has never been. the folder is conf/services and it is unchanged from openHAB 2. So the configuration is just done as before, no changes to that.

@digitaldan
Copy link
Contributor

configuration is just done as before, no changes to that.

Trying use our new UI in a config file free use case made me realize we do not have the ability to configure persistence services through our API's, is this correct? Do you think this will be needed for 3.0 ?

@ssalonen
Copy link
Contributor

ssalonen commented May 9, 2020

I will try to place the file to old place and report back.

I think we used to have PaperUi UI for dynamodb persistence configuration but not anymore with 3.0 admin UI?

EDIT: I have trouble enabling persistence with openHAB3.0 :(

This is what I have configured for the demo app:

$ cat ~/src/openhab-master/git/openhab-distro/launch/app/runtime/conf/services/dynamodb.cfg                                                                               
service.pid="org.openhab.dynamodb"
accessKey=AKIAIOSFODNN7EXAMPLE
#secretKey=3+AAAAABBBbbbCCCCCCdddddd+7mnbIOLH
region=eu-west-1
tablePrefix=oh3-

I get no log entries at all (log4j configured with DEBUG root logger). I would expect error due to missing key secretKey

The shell does not seem to have the usual bundle:list command available so I'm not sure how to debug whether everything is picked up correctly...

Is there admin UI for configuring the persistence addon similar to PaperUI? DynamoDB should have the description for the UI (config.xml)

@ghys
Copy link
Member

ghys commented May 10, 2020

Is there admin UI for configuring the persistence addon similar to PaperUI?

What does it look like in Paper UI? I have never seen it.

@ssalonen
Copy link
Contributor

@ghys
This is how it looks like:
Screenshot from 2020-05-10 19-49-12

@ghys
Copy link
Member

ghys commented May 10, 2020

So that's a service. It should then appear under "other services" in the main settings menu.

@ssalonen
Copy link
Contributor

thanks @ghys. Unfortunately I do not see it there, only "Rule voice interpreter" and "Basic UI"

I have added the dynamodb persistence bundle in Run requirements of the demo app, nothing else.

As mentioned earlier, I'm not sure whether the bundle is loaded or not, since the shell does not seem to have the usual commands to diagnose running bundles...

@ghys
Copy link
Member

ghys commented May 10, 2020

I've done the same, installed the DynamoDB add-on, and the configurable service should appear in the call to /rest/services but I don't see it...

image

I'd be interesting to check if it's the case in 2.5.x so we can pinpoint what the actual issue is.

@ghys
Copy link
Member

ghys commented May 10, 2020

Gave it a try on the latest 2.5.5 snapshot, and the service does indeed appear:

image

So there's something wrong in 3.0 wrt to these services, when it's fixed it will appear in the UI.

@wborn
Copy link
Member

wborn commented May 10, 2020

The ESH-INF dirs should be renamed to OH-INF and they should be in src/main/resources:

$ find . | grep ESH-INF 
./bundles/org.openhab.persistence.influxdb/src/main/resources/ESH-INF
./bundles/org.openhab.persistence.influxdb/src/main/resources/ESH-INF/config
./bundles/org.openhab.persistence.influxdb/src/main/resources/ESH-INF/config/config.xml
./bundles/org.openhab.persistence.dynamodb/ESH-INF
./bundles/org.openhab.persistence.dynamodb/ESH-INF/config
./bundles/org.openhab.persistence.dynamodb/ESH-INF/config/config.xml

I'll test if that fixes it and create a PR for it.

wborn added a commit to wborn/openhab-addons that referenced this pull request May 10, 2020
* Update config.xml file paths
* Add missing annotations to DynamoDBPersistenceService
* Remove required configuration policy from DynamoDBPersistenceService causing the component to be not instantiated

See: openhab#5275 (comment)

Signed-off-by: Wouter Born <github@maindrain.net>
@kaikreuzer
Copy link
Member Author

Oops, seems if I had missed those when migrating, sorry...
Thanks for fixing it. @wborn!

kaikreuzer pushed a commit that referenced this pull request May 10, 2020
* Update config.xml file paths
* Add missing annotations to DynamoDBPersistenceService
* Remove required configuration policy from DynamoDBPersistenceService causing the component to be not instantiated

See: #5275 (comment)

Signed-off-by: Wouter Born <github@maindrain.net>
@ssalonen
Copy link
Contributor

Thank you all. Can confirm that UI & file works, success with persistence store and query.

Posted #7598 for some small improvements (mainly related to java8 -> java11)

@@ -16,6 +16,17 @@

<name>openHAB Add-ons :: Features :: Karaf</name>

<properties>
<!-- JDBC database driver versions -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaikreuzer Can you briefly if there was a special reason for adding the JDBC version properties in this file? It created confusion in #9810 (comment).

Can we remove them? Or add them in the in the feature file of the bundle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall, sorry. If it works without it, we can remove them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code compiles successfully when I remove the properties from this file. I did not test installing the features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.