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

There should be an additive option for decorating ExecutorServices, potentially outside the Factory #1408

Closed
simonbasle opened this issue Oct 25, 2018 · 4 comments
Assignees
Labels
type/enhancement A general enhancement warn/deprecation This issue/PR introduces deprecations
Milestone

Comments

@simonbasle
Copy link
Member

Libraries like Sleuth use the Schedulers.Factory to decorate the backing ScheduledExecutorService, rather than changing the Scheduler implementations themselves.

Now that we want to introduce a thin Micrometer instrumentation wrapper (#1201) that would work on the same ExecutorServices, it appears this can be problematic. It has also proven error-prone when user code captures a Scheduler before a library sets a Factory for the purpose of ExecutorService-wrapping, without actually touching the Schedulers, because setFactory always shuts down previous schedulers 😢 (see spring-cloud/spring-cloud-sleuth#866)

As a result, it would be far better to have Schedulers.addDecorator and Schedulers.removeDecorator type of methods, which would allow additive set up of ExecutorService decorators separately from the Scheduler Factory.

@simonbasle simonbasle added type/enhancement A general enhancement warn/behavior-change Breaking change of publicly advertised behavior labels Oct 25, 2018
@simonbasle simonbasle added this to the 3.2.2.RELEASE milestone Oct 25, 2018
@simonbasle simonbasle self-assigned this Oct 25, 2018
@simonbasle
Copy link
Member Author

The current Supplier<ScheduledExecutorService> model doesn't work well with composition, but it had the additional benefit of hiding a sort of "metadata" in the toString() method of the suppliers => the Scheduler themselves are the Supplier and thus they define a rich toString() method. It would be good to add the notion of metadata to the new method in addition to the "Scheduler type" String.

@simonbasle simonbasle added warn/deprecation This issue/PR introduces deprecations and removed warn/behavior-change Breaking change of publicly advertised behavior labels Oct 25, 2018
@simonbasle
Copy link
Member Author

proposed API: public static void addExecutorServiceDecorator(BiFunction<Tuple2<String, String>, ScheduledExecutorService, ScheduledExecutorService> decorator)

The legacy Factory decorateExecutorService would be deprecated, but still applied first when a Scheduler calls Schedulers.decorateExecutorService.

@smaldini smaldini modified the milestones: 3.2.2.RELEASE, 3.2.3.RELEASE Oct 25, 2018
@simonbasle simonbasle changed the title There should be an additive option for decorating ExecutorServices, outside the Factory There should be an additive option for decorating ExecutorServices, potentially outside the Factory Oct 25, 2018
@simonbasle
Copy link
Member Author

after discussing with @smaldini and @marcingrzejszczak, maybe a composite addFactory API would be better, and in any case keying the decorator with a String would be preferred vs keeping up with instances.

@madgnome
Copy link
Contributor

As a consumer, I would indeed prefer working with String key than having to keep a BiFunction around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement warn/deprecation This issue/PR introduces deprecations
Projects
None yet
Development

No branches or pull requests

3 participants