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

feat: ServiceConfigurator support for various service configurations #3826

Merged

Conversation

ikaakkola
Copy link
Contributor

@ikaakkola ikaakkola commented Jan 23, 2024

Add support for ServiceConfigurator to Variable, Task, IdentityLink, EntityLink, EventSubscription and Batch ServiceConfiguration to allow customisation at init time.

The implementation follows the logic of JobServiceConfiguration that already has support for ServiceConfigurator.

Check List:

  • Unit tests: YES
  • Documentation: NA

@ikaakkola
Copy link
Contributor Author

@filiphr related to discussion in #3825 , these allow proper configuration of the DataManagers via ServiceConfigurator interface (identical to JobServiceConfiguration was already implemented)

@ikaakkola
Copy link
Contributor Author

ikaakkola commented Jan 24, 2024

To add more detail to why this is required, taking "VariableServiceConfiguration" as an example, the exact same applies to all the ServiceConfigurations improved with this PR.

VariableServiceConfiguration is initialized here https://github.com/flowable/flowable-engine/blob/main/modules/flowable-engine/src/main/java/org/flowable/engine/impl/cfg/ProcessEngineConfigurationImpl.java#L981 which ends up calling https://github.com/flowable/flowable-engine/blob/main/modules/flowable-variable-service/src/main/java/org/flowable/variable/service/VariableServiceConfiguration.java#L78-L82 - after "configuratorsBeforeInit()" , so if one tries to access VariableServiceConfiguration in beforeInit to set the DataManager, it will fail as it has not yet been instantiated.

The init() method of VariableServiceConfiguration initialises DataManagers and EntityManagers without any way of setting a custom DataManager before the EntityManager is also initialized.
Thus, to have a custom DataManager one also needs to setVariableInstanceEntityManager(manager) with a new entity manager instance (one that uses the custom DataManager), because dataManager within the default VariableInstanceEntityManager interface is immutable (my previous approach at this made dataManager mutable, but that was not a clean approach).

So to be able to initialize the standard VariableServiceConfiguration with the standard VariableInstanceEntityManagerImpl but with a custom DataManager, one needs a way to set DataManager inside VariableServiceConfiguration before the entity manager is initialized - that is accomplished with ServiceConfigurator , exactly as JobServiceConfiguration already does it.

I believe this is the cleanest and most maintainable approach to the problem especially because it simply mimics what has already been implemented for JobServiceConfiguration.

@filiphr
Copy link
Contributor

filiphr commented Jan 24, 2024

Thanks @ikaakkola. This one makes a lot of sense. It also looks good for merging. I am only contemplating if we can move something to the AbstractServiceConfiguration and avoid the duplication we have for

 List<ServiceConfigurator<BatchServiceConfiguration>> configurators;
        if (this.configurators != null) {
            configurators = new ArrayList<>(this.configurators);
            configurators.sort(Comparator.comparingInt(ServiceConfigurator::getPriority));
        } else {
            configurators = Collections.emptyList();
        }

        for (ServiceConfigurator<BatchServiceConfiguration> configurator : configurators) {
            configurator.beforeInit(this);
        }

Will talk with the team and merge this soon

@ikaakkola
Copy link
Contributor Author

I agree the configurator logic is very repetitive, it could probably be generalized in the abstract class.

@filiphr
Copy link
Contributor

filiphr commented Jan 24, 2024

@ikaakkola I talked with the team and we think that it would be better if we can move this repetitive bits in the AbstractEngineConfigurator. I made a small spike in 9cbde32 to see how it can look like. Feel free to use it as an inspiration.

@ikaakkola
Copy link
Contributor Author

@filiphr added generic ServiceConfigurator support for AbstractServiceConfiguration and implemented it in the concrete classes.

I also considered moving the before and after init calls to a AbstractServiceConfiguration (so one would not forget to call the configuratorsBefore/AfterInit), something like:

    protected void init(Consumer<S> serviceInit) {
        configuratorsBeforeInit();
        serviceInit.accept(getService());
        configuratorsAfterInit();
    }

and in concrete ServiceConfiguration class call like:

    public void init() {
        init( s -> {
            s.initDataManagers();
            s.initEntityManagers();
        });
    }

but this doesn't seem to be a pattern used in the code base, so opted not to introduce it for now.

@ikaakkola ikaakkola force-pushed the feature/additional-service-configurators branch 2 times, most recently from 2ebd6c1 to 199675f Compare January 25, 2024 08:04
@filiphr
Copy link
Contributor

filiphr commented Jan 25, 2024

I also considered moving the before and after init calls to a AbstractServiceConfiguration (so one would not forget to call the configuratorsBefore/AfterInit), something like:

That looks interesting. However, we are indeed not using something like that in the codebase. Perhaps at some point down the line. I'll go through your changes now

Copy link
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

Thanks @ikaakkola. Looks good. I've left some comments, some small cleanups before we merge this.

Let me know if you don't agree with something.

@ikaakkola ikaakkola force-pushed the feature/additional-service-configurators branch from 199675f to bdb70cf Compare January 25, 2024 10:15
@ikaakkola ikaakkola requested a review from filiphr January 25, 2024 10:18
Copy link
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

@ikaakkola I've missed one place for the formatting and the JobServiceConfiguration still has the configurators field, I think we should remove that as well.

…tion

Added a generic implementation of ServiceConfigurator to AbstractServiceConfiguration and implemented it in the concrete ServiceConfiguration classes.

This change allows easier customisation of the individual ServiceConfigurations while they are initialized.
@ikaakkola ikaakkola force-pushed the feature/additional-service-configurators branch from bdb70cf to fd1d5a4 Compare January 25, 2024 10:36
Copy link
Contributor

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes and contribution @ikaakkola. Will merge this once the tests are green

@filiphr filiphr merged commit 1b0e465 into flowable:main Jan 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants