-
-
Notifications
You must be signed in to change notification settings - Fork 457
Persistence no default strategies and persistence configuration health check #4682
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
base: main
Are you sure you want to change the base?
Conversation
@J-N-K Can you let me know what you think? If this is accepted as the direction, I could start working on the UI side of this. But I don't want to spend time if the direction is not agreed upon. |
This is what I suggested about two years ago (IIRC). IMO a "proposal" is much better than some magic that is applied. However, there might be an issue with the "empty strategy". It would be better (but not so easy to implement) when the persistence service is not considered "ready" up without a valid configuration. It is necessary to allow the service to start, otherwise it can't be found and as a result the suggested strategy is not present (and the service can't be configured), but it should not be considered as "available". |
Yes, I agree that would be better. But it would require implementing a life cycle for persistence services (like for things) which is beyond what I feel comfortable doing. A persistence service persisting anything is not just dependent on a strategy being available, but also items to be persisted defined. What if you delete the all item definitions from an existing configuration? Should it go back to not ready? It should. I think it makes it more complicated then needed, and just not doing anything with an empty definition, or an item definition with no strategy is acceptable I believe. |
1666ab4
to
0f0b1c6
Compare
0301274
to
773395a
Compare
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
@holgerfriedrich I saw #4693. This PR is already rebased on that PR, but I still see a javadoc failure. Should this be resolved? |
* be used to allow the consumer (e.g. UI) to tailor the message. | ||
* @param serviceId persistence service | ||
* @param items list of persistence item definitions | ||
* @param strategy persistence strategy selected for the list of items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param strategy persistence strategy selected for the list of items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good now I believe.
@mherwege the log of the javadoc job needs improvement. Running it locally, I see the message causing it to fail:
|
Thanks. How do I run this locally? |
3eaea1b
to
709e40b
Compare
What is the current state here? I see 4/4 tasks completed, but has everything been done and this can be considered as "ready for review"? |
Yes, I consider this ready for review, in combination with the related PR's that are also ready. |
@J-N-K Any chance to get this across the line? |
709e40b
to
f72a54e
Compare
Rebased to account for changes in UpgradeTool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a breaking change that removes automatic application of default persistence strategies when no strategy is defined, requiring explicit configuration by users instead.
Key changes:
- Deprecated
PersistenceService.getDefaultStrategies()
in favor ofPersistenceService.getSuggestedStrategies()
- Added persistence configuration health check API endpoint
- Created upgrade tool to migrate existing configurations with missing strategies
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
tools/upgradetool/src/main/java/org/openhab/core/tools/internal/PersistenceUpgrader.java | New upgrader class to migrate persistence configurations by copying default strategies to configurations without explicit strategies |
tools/upgradetool/src/main/java/org/openhab/core/tools/UpgradeTool.java | Added persistence upgrader to the list of upgraders |
tools/upgradetool/pom.xml | Added persistence bundle dependency and OSGi annotations |
bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceService.java | Deprecated getDefaultStrategies() and added getSuggestedStrategies() method |
bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/PersistenceServiceProblem.java | New record class for representing persistence configuration problems |
bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/internal/PersistenceManagerImpl.java | Removed automatic application of default strategies and simplified configuration handling |
bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/persistence/PersistenceResource.java | Added new REST endpoints for strategy suggestions and persistence health checks |
tools/upgradetool/src/main/java/org/openhab/core/tools/internal/PersistenceUpgrader.java
Show resolved
Hide resolved
...tence/src/main/java/org/openhab/core/persistence/dto/PersistenceServiceConfigurationDTO.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/org/openhab/core/io/rest/core/internal/persistence/PersistenceResource.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/org/openhab/core/io/rest/core/internal/persistence/PersistenceResource.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
3595f9e
to
11cc97c
Compare
@wborn Rebased and copilot suggestions considered. |
Closes #4470
This is a breaking change.
With this PR, the default persistence strategy is not applied automatically anymore when no other strategy is defined.
The PersistenceService interface still keeps the getDefaultStrategies method. Therefore no change is required in the persistence services implementations. But rather than applying this automatically, these default strategies are now considered configuration suggestions.The current
PersistenceService.getDefaultStrategies()
method has been deprecated and replaced by aPersistenceService.getSuggestedStrategies()
method. A default implementation will redirectPersistenceService.getSuggestedStrategies()
toPersistenceService.getDefaultStrategies()
, so there is no need to immediately changePersistenceService
implementations.An extra sub-endpoint to the
persistence
REST API endpoint is defined to retrieve these strategy suggestions (GET persistence/strategysuggestions?serviceId={serviceId}
). This endpoint can be used by the UI to suggest strategies when configuring the service. Applying these suggestions will then become a clear user decision.File based persistence configurations will have to be changed by the user if they use the default strategies.
As part of this PR, a persistence configuration health check endpoint has been implemented(
GET persistence/persistencehealth?serviceId={serviceId}
). This endpoint will then be used in the UI to show persistence configuration health (similar to orphan links), and will allow more easy support when default strategies are being removed.To complete this functionality, the following is also needed: