Repository navigation
Conversation
- PostStartupConfig log message no reflects that only the process engine JobExecutor is started and not the entire (already running) process engine. - Level of JettyServer ErrorHandler generateResponse log message reduced from info to debug. Some 5xx status codes are not implementation errors.
This breaking change to the config parameters needs to be mentioned in the release notes.
- New health check abstraction with implementation for database connection check and websocket connection open check. - Websocket connection open check can be disabled via config property. - Modified LocalFhirConnectorImpl to except an Executor (connectExecutor) as constructor parameter in order to reuse the (single thread) executor for websocket reconnects. The old implementation had a thread resource leak, creating new single thread pools every time the websocket connection failed.
- Moved checks from InitializingBean.afterPropertiesSet methods to constructors and removed InitializingBean implementation declarations where possible. - Removed some not needed class fields and related constructor parameters. - Refactored some thread executor related code. - Added constructor parameter documentation (JavaDoc). - Improved config parameter documentation. - Changes to some tests to reflect removed InitializingBean implementations.
issue/594_Websocket_Connection_Status_in_BPE_Status_Webservice
issue/594_Websocket_Connection_Status_in_BPE_Status_Webservice
schwzr
reviewed
Oct 26, 2026
| * @see ProcessPluginDefinition#getSpringConfigurations() | ||
| */ | ||
| public abstract class AbstractServiceDelegate implements JavaDelegate, InitializingBean | ||
| public abstract class AbstractServiceDelegate implements JavaDelegate |
Member
There was a problem hiding this comment.
I like the idea of removing the afterPropertiesSet methods in the DSF. However, we might get some regression errors on APIv1 (maybe even APIv2), when PP devs used it in their PP? E.g., they might have an override on afterPropertiesSet with a super.afterPropertiesSet() call that might break with the new DSF version?
So we might only remove the method content, not the method itself, escpecially for classes in the PP API?
| @Override | ||
| public boolean isConnected() | ||
| { | ||
| return connection != null && connection.isOpen(); |
Member
There was a problem hiding this comment.
This connection might not the the "current" connection. In https://github.com/datasharingframework/dsf/pull/595/changes#diff-a4798d4e0a1e2908c34ed52e0ec23c8ae8390ff344b37233c9b93fd5ffc558b3L96 we request a new connection attempt, but in
we don't store it as a new connection. So we might restart the application through a healthcheck while it is healthy, just because we stored the old connection and not the updated one?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New health check abstraction with implementation for database connection check and websocket connection open check.
Breaking change: Renamed config parameters / environment variables. Needs to be mentioned in the release notes.
dev.dsf.bpe.fhir.task.subscription.search.parameter->dev.dsf.bpe.fhir.subscription.task.search.parameterdev.dsf.bpe.fhir.questionnaire.response.subscription.search.parameter->dev.dsf.bpe.fhir.subscription.questionnaireresponse.search.parameterdev.dsf.bpe.fhir.task.subscription.retry.max->dev.dsf.bpe.fhir.subscription.retry.maxdev.dsf.bpe.fhir.task.subscription.retry.sleep->dev.dsf.bpe.fhir.subscription.retry.sleepWebsocket connection open check can be disabled via config property:
dev.dsf.bpe.fhir.subscription.health.checkModified
LocalFhirConnectorImplto except an Executor (connectExecutor) as constructor parameter in order to reuse the (single thread) executor across websocket reconnects. The old implementation had a thread resource leak, creating new single thread pools every time the a websocket connection failed.Moved checks from
InitializingBean.afterPropertiesSetmethods to constructors and removedInitializingBeanimplementation declarations where possible. Doing simple parameter checks inafterPropertiesSetwas a bad design. Distance to parameter declaration too large and not usable for parameters that need to be accessed in the constructor.Improved config parameter documentation.
closes #594