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

Allow managing persistence configurations and enable filters #2871

Merged
merged 5 commits into from
May 7, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Mar 25, 2022

Depends on #2994
Fixes #3590

The aim of this PR is to allow managing persistence service configurations via the REST API (UI).

To achieve this

  • A PersistenceServiceConfigurationRegistry has been introduced. It needs to add an own interface for the change listener because one class can't inherit two RegistryChangeListener<T> with different types and the persistence manager needs to listen to both, the ItemRegistry and the PersistenceServiceConfigurationRegistry.
  • The PersistenceManager interface has been removed.
  • The PersistenceManagerImpl has been renamed to PersistenceManager and makes use of the new registry.
  • The PersistenceModelManager has been refactored to a PersistenceServiceConfigurationProvider.
  • A ManagedPersistenceServiceConfigurationProvider has been introduced.
  • The core REST bundle has been extended to allow adding configurations. Unmanaged configurations can also be retrieved.

Signed-off-by: Jan N. Klug [email protected]

@J-N-K J-N-K requested a review from a team as a code owner March 25, 2022 19:00
@J-N-K
Copy link
Member Author

J-N-K commented Mar 26, 2022

@ghys Can you have a look at the REST part and let me know if that is a good design for the UI?

@wborn wborn added the enhancement An enhancement or new feature of the Core label Mar 26, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Mar 26, 2022

I'll add more extensive test coverage to ensure that my refactoring are all correct.

@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from d0dba2d to b360c1f Compare March 27, 2022 13:59
@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from c56d40c to d78ed90 Compare April 2, 2022 12:53
@splatch
Copy link
Contributor

splatch commented Apr 7, 2022

Since you touch fairly important base - have a look on PersistenceFilter which is blind interface without use. While it can be referred in persistence service config it plays no role in config itself. I made it an Predicate<Item> which allows to use it as an exclusion filter for certain items. Obviously there is still a problem of mapping the filters, but that's other thing. Other elephant then would be the xtext part which would need to be provided by someone who knows how to work with it (or omitted as deprecated).

@J-N-K J-N-K force-pushed the feature-persistenceregistry branch 2 times, most recently from 549e233 to 22e6de1 Compare April 18, 2022 11:48
@J-N-K J-N-K changed the title Allow managing persistence configurations Allow managing persistence configurations and enable filters Apr 18, 2022
@J-N-K J-N-K force-pushed the feature-persistenceregistry branch 4 times, most recently from 4bf6efe to 78575a8 Compare April 20, 2022 16:21
@J-N-K
Copy link
Member Author

J-N-K commented Apr 22, 2022

@splatch I enabled the PersistenceFilter.

@splatch
Copy link
Contributor

splatch commented Apr 22, 2022

@splatch I enabled the PersistenceFilter.

Yes, I saw it and see you use it to throttle or limit writes to persistence stuff. It makes a lot of sense cause this can be used when multiple stores are used. My approach so far was focusing on use of profiles which worked for single persistence but not for all.

I think there is still an area for improvement in excluding certain items, cause mapFilter is currently fairly limited. Its really about use cases reported several times in forums (where I advertised own externalized XML config for that) to serve cases where it is easier to mark item with an tag than create a completely new group without just few members.
Knowing the limit of xtext I think it could be done with a basic instanceof call to see if ie. incoming filter instance is already compatible with contract and does not require further mapping.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 22, 2022

The two filters I added were already defined in the model, that's why I implemented them. If someone knows how to properly extend that, more filters can be added. Maybe something like !itemName could be used to exclude items. But it's difficult if we need wildcards. This should ne done in a separate PR. I only added the filters here because I needed to figure out how to model them for the REST API.

@splatch
Copy link
Contributor

splatch commented Apr 22, 2022

@J-N-K I'm perfectly fine with your work, my stuff is sitting on OH 3.0.x fork, so I still have plenty of time to fight with 3.3 updates. ;-)

@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from 8121a8f to f93e881 Compare May 16, 2022 21:32
@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from f93e881 to a94fffb Compare August 3, 2022 16:02
@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/is-there-a-way-to-limit-the-precision-of-number-items/141006/7

@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from df77f51 to e4cc100 Compare December 25, 2022 20:38
@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from e4cc100 to df3bd28 Compare January 3, 2023 20:38
@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/profiling-results-using-jprofiler/143073/2

@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/openhab-4-0-wishlist/142388/248

@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from df3bd28 to dfde419 Compare April 7, 2023 18:50
@J-N-K
Copy link
Member Author

J-N-K commented Apr 7, 2023

@florian-h05 FYI. This is also lacking UI support and I don't have a good idea how this should look like.

@florian-h05
Copy link
Contributor

Thanks for the hint, I have also seen openhab/openhab-webui#1463.
I am currently super busy wit my final exams (Abitur), but I put this on my UI list 👍

@clinique
Copy link
Contributor

Abitur

May the force be with you !

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

I am currently super busy wit my final exams (Abitur), but I put this on my UI list 👍

Best of luck!

J-N-K added 3 commits May 4, 2023 16:39
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K J-N-K force-pushed the feature-persistenceregistry branch from dfde419 to 5a17529 Compare May 4, 2023 15:04
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Many thanks! I have just a few small comments below.

J-N-K added 2 commits May 7, 2023 19:11
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks again!

@kaikreuzer kaikreuzer added this to the 4.0 milestone May 7, 2023
@kaikreuzer kaikreuzer merged commit 2e00efc into openhab:main May 7, 2023
@J-N-K J-N-K deleted the feature-persistenceregistry branch May 7, 2023 18:51
@lolodomo
Copy link
Contributor

Other elephant then would be the xtext part which would need to be provided by someone who knows how to work with it (or omitted as deprecated).

@J-N-K : What exactly is missing in current XText for persistence?
I can try to add/fix it if I understand what is the problem.

@J-N-K
Copy link
Member Author

J-N-K commented May 18, 2023

Filters are already enabled (I think we need documentation for that). Currently there are two types of filters: value-based and time-based. If we would add more, then there would also be the need for XText support, but I think currently we are fine.

@lolodomo
Copy link
Contributor

Fine.
I see them in XText syntax.

ghys pushed a commit to openhab/openhab-webui that referenced this pull request Jun 28, 2023
Closes #1463.
Refs openhab/openhab-core#2871.
Refs openhab/openhab-core#3642.

It is accessible from the add-on settings page and has both a design and
a code tab.

The design tab allows to set persistence strategies for Items, define
cron strategies and set the default strategies. It does not duplicate
names for (cron) persistence strategies and filters as well as configs
for the same set of Items.
All four filters provided by openHAB core (treshold, time, equals/not
equals, include/exclude) can be configured.
When the user removes a cron strategy or a filter, it is automatically
removed from all configs so that there is no API failure (400 Bad
Request).

No code completion is not provided, but required attributes for filters
are automatically set on save to avoid API failure (500 Internal Server
Error).

A few words about order and sorting:
- openHAB Core seems to sort the cron strategies.
- Configurations itself are unsorted, they could be sorted
alphabetically by the UI.
- Items of configuration are sorted by their type (groups before normal
Items) as well as alphabetically.

--
Signed-off-by: Florian Hotze <[email protected]>
Co-authored-by: J-N-K <[email protected]>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
…#2871)

* Allow managing persistence configurations

Signed-off-by: Jan N. Klug <[email protected]>
GitOrigin-RevId: 2e00efc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core persistence
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Change behavior when .persist file is reloaded or when starting OH if no .persist file is loaded yet.
8 participants