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

Add a PersistenceService bundle tracker #4324

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jul 17, 2024

While working on adding time series support to the Awattar binding I found that - depending on the installation order - things might get initialized before all persistence services are ready. It might happen that this leads to issues with persisting data (because a state or time series is sent before the persistence service is ready).

I propose to add a persistence service bundle tracker (similar to what we have for script engine factories) and prevent early thing initialization. Adding the new persistence=services ready marker as a requirement to start level 30 and delaying thing initialization until that level is reached makes sure that persistence services are ready.

Increasing the minimum start level for thing initialization also solves another issue reported on the forum: things initialized before scripting add-ons might result in warnings that a script engine factory is missing for the SCRIPT transformation used in profiles.

There might be a short delay in startup but IMO the benefits outweigh that - and maybe it doesn't influence the total startup time that much because the scripting add-ons and the persistence add-ons are just started earlier than before. They are now using more ressources during early startup but less on later levels.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Jul 17, 2024
@J-N-K J-N-K requested a review from a team as a code owner July 17, 2024 16:30
@lolodomo
Copy link
Contributor

lolodomo commented Jul 18, 2024

By ready, does it mean also with user persistence configuration loaded ?
You certainly remember we have an annoying bug that it can happen items are persisted before the user persistence configuration is loaded, leading to all items being persisted during this short time while the default persistence config is used. Does your change also resolve this problem, or reduce it or make it even worst or change nothing?

@J-N-K
Copy link
Member Author

J-N-K commented Jul 18, 2024

It probably reduces the problem, because the startup order is more deterministic and the short time where the "wrong" configuration is applied is moved to a point where no item updates are sent.

But as long as the "default persistence strategy" is allowed (and there was a lengthy discussion about that here where everybody except myself voted for keeping and even extending them) the issue will not go away. The only way to really solve it would be to remove the default strategies and wait for the selected configuration to be applied - the service can't decide if the default strategy is what is used intentionally or if another configuration is just not yet loaded.

@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/there-is-no-queryable-persistence-service-registered-with-the-id-jdbc/157713/4

@apella12
Copy link

This PR sounds like a good idea. I did get some startup messages on my latest update to 4.2.1 that I don't recall from before. No issues observed after a few seconds. OH in docker on RPI5. Included a few surrounding messages for context in case that helps.

2024-08-09 21:11:49.220 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'Sensors.items'
2024-08-09 21:11:49.259 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'Light_count.items'
2024-08-09 21:11:49.285 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'house-mode.items'
2024-08-09 21:11:55.960 [INFO ] [e.automation.internal.RuleEngineImpl] - Rule engine started.
2024-08-09 21:12:03.980 [INFO ] [.network.internal.utils.NetworkUtils] - CIDR prefix is smaller than /24 on interface with address 172.29.0.1/16, truncating to /24, some addresses might be lost
2024-08-09 21:12:05.153 [INFO ] [tocol.googletv.GoogleTVMessageParser] - E89EB43854BA - Login Successful
2024-08-09 21:12:07.222 [INFO ] [ternal.dhcp.DHCPPacketListenerServer] - DHCP request packet listener online
2024-08-09 21:12:10.028 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:10.029 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:10.215 [INFO ] [.transport.mqtt.MqttBrokerConnection] - Starting MQTT broker connection to '192.168.0.186' with clientid 2194bf2a-3d92-44aa-a540-1763ec44e2bc
2024-08-09 21:12:11.003 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:11.003 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:13.006 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:13.006 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:15.008 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:15.009 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:17.013 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:17.014 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:19.017 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:19.017 [WARN ] [nce.extensions.PersistenceExtensions] - There is no queryable persistence service registered with the id 'rrd4j'
2024-08-09 21:12:20.440 [INFO ] [persistence.jdbc.internal.JdbcMapper] - JDBC::openConnection: Driver is available::Yank setupDataSource

@florian-h05
Copy link
Contributor

But as long as the "default persistence strategy" is allowed (and there was a lengthy discussion about that openhab/openhab-addons#16496 where everybody except myself voted for keeping and even extending them) the issue will not go away.

Just to mention it, I did only vote for the default strategy because I did not see a good alternative.
I though a bit more about that, what if we change the default strategy to a „proposed“ strategy, which the UI can query (as with the current default strategy) and when installing persistence through the wizard, the UI could then get that „proposed“ strategy and save it.
This way we would get rid of the default strategy whilst keeping an easy setup — existing users then would only need to open the persistence config in the UI and click on save to migrate that „breaking“ change.

@J-N-K Please let me know what you think.

@mherwege
Copy link
Contributor

mherwege commented Aug 15, 2024

I though a bit more about that, what if we change the default strategy to a „proposed“ strategy, which the UI can query (as with the current default strategy) and when installing persistence through the wizard, the UI could then get that „proposed“ strategy and save it.

Fully agree, and that is also what I proposed when this was originally discussed.
I think that neatly solves it when persistence is configured through the UI. It may even be possible to create an upgrade routine that automatically creates the config if no config is already set. The challenge would be with text file configured persistence. And that is probably still the majority. I can see the number of questions on the forum about persistence not working anymore arriving.

See openhab/openhab-addons#16496 (comment)

@florian-h05
Copy link
Contributor

The challenge would be with text file configured persistence.

I don‘t think necessarily. If there is a config file, everything is fine. If there is no config file and no managed config (which I think core should be able to detect), we can use the recommended config and store it as a new managed config. Do you agree or do I have a misconception?

@mherwege
Copy link
Contributor

I don‘t think necessarily. If there is a config file, everything is fine. If there is no config file and no managed config (which I think core should be able to detect), we can use the recommended config and store it as a new managed config. Do you agree or do I have a misconception?

I need to think this through a bit more. If you upgrade and there is no file, it can be created automatically as managed configuration. But the user then needs to remove it again and create a file version if he/she prefers that (and wants to change it). I guess we may end up in situations where there is a managed config and a file config at the same time. While I like the idea of automatically creating a managed config, I guess this is an area where we risk having issues with users creating file configs afterwards. The answer is: you shouldn’t, and only one config is allowed, but still. So maybe we should not do it automatically after all and have update instructions stating no default is applied, and it can be created in a file or by saving the proposed config in the UI.

@florian-h05
Copy link
Contributor

So maybe we should not do it automatically after all and have update instructions stating no default is applied, and it can be created in a file or by saving the proposed config in the UI.

Maybe true, but I can already see a few users writing on the forum „my persistence stopped working“ because they didn‘t read the release notes/breaking changes section no matter how often we tell them to do … I guess this just happens.
I will try to think a bit more about that as well, would be great to get this resolved with openHAB 4.3.

And sorry for getting off-topic here …

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Nice RFC of the bundle tracker, and I consider it always a good thing to make startup more determinist.

Just one minor comment from my side.

@rkoshak
Copy link

rkoshak commented Aug 19, 2024

I guess we may end up in situations where there is a managed config and a file config at the same time.

For all other configs, the file takes precedence. I would expect this to be the same. If a file exists, it's marked as "read only" so MainUI doesn't allow editing of it and anything that may have been done in MainUI prior gets masked by the file.

@J-N-K
Copy link
Member Author

J-N-K commented Aug 19, 2024

@rkoshak I think that's not something you should place a bet on. There is no precedence in core, the first one wins. Maybe this is textual config on most cases, but it is not guaranteed.

In general: there should be no "automatic managed configuration". The "default" could be suggested and be set as managed if accepted, but that should not happen without user interaction.

@rkoshak
Copy link

rkoshak commented Aug 19, 2024

It used to be the case that the file always took precidence. Maybe it was never that cut and dry (though we were told that it was at the time) or maybe things are different now. This goes back to OH 2 times so it might have become less determinisitic than it once was. Or maybe we've been lucky becuase loading and parsing files is so much more slow than loading managed configs?

…e/AbstractServiceBundleTracker.java

Co-authored-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: J-N-K <github@klug.nrw>
Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

LGTM!

@holgerfriedrich holgerfriedrich merged commit 53889cd into openhab:main Sep 9, 2024
5 checks passed
@holgerfriedrich holgerfriedrich added this to the 4.3 milestone Sep 9, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants