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

Allow disabling all Callback decorators via environment properties #670

Merged
merged 1 commit into from Sep 1, 2021

Conversation

Sanne
Copy link
Contributor

@Sanne Sanne commented Aug 31, 2021

The CallbackDecorator provided by Contex Propagation has a strong impact on performance, and in some contexts unnecessary.

It's rather tricky to remove it from classpath, so it would be very handy to have a different option to disable it.

cc/ @jponge @FroMage

@FroMage
Copy link
Contributor

FroMage commented Aug 31, 2021

I feel that it could be useful to have a more general approach to disabling ServiceLoader stuff, perhaps with a setting that overrides the list of detected services? This way we could use this approach in more loaded service lists, and perhaps exclude specific services on top of being able to disable them all?

Something like services.fqdn.CallbackDecorator= to disable it, and services.fqdn.CallbackDecorator=foo.BarDecorator,gee.OtherDecorator?

I've needed something like this for Quarkus sometimes…

@Sanne
Copy link
Contributor Author

Sanne commented Aug 31, 2021

I feel that it could be useful to have a more general approach [...]

I agree, butl all this state is currently a global static. That would need some more extensive refactoring which might not be suitable do do now, while I would need a solution in short term.

@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #670 (78315b6) into main (e06ce7d) will decrease coverage by 0.06%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #670      +/-   ##
============================================
- Coverage     90.25%   90.19%   -0.07%     
- Complexity     2989     2992       +3     
============================================
  Files           374      374              
  Lines         11762    11764       +2     
  Branches       1476     1478       +2     
============================================
- Hits          10616    10610       -6     
- Misses          584      591       +7     
- Partials        562      563       +1     
Impacted Files Coverage Δ
...smallrye/mutiny/infrastructure/Infrastructure.java 73.03% <85.71%> (-0.27%) ⬇️
...utiny/operators/multi/MultiWindowOnDurationOp.java 65.71% <0.00%> (-5.72%) ⬇️
...java/io/smallrye/mutiny/helpers/Subscriptions.java 79.00% <0.00%> (-2.77%) ⬇️
...perators/multi/processors/SerializedProcessor.java 90.09% <0.00%> (-1.99%) ⬇️
...mallrye/mutiny/operators/multi/MultiFlatMapOp.java 85.98% <0.00%> (-0.63%) ⬇️
...mallrye/mutiny/helpers/queues/MpscLinkedQueue.java 98.38% <0.00%> (ø)
...erators/multi/builders/SerializedMultiEmitter.java 83.54% <0.00%> (+1.26%) ⬆️
...tiny/operators/multi/MultiBufferWithTimeoutOp.java 74.78% <0.00%> (+1.68%) ⬆️
...y/operators/multi/processors/UnicastProcessor.java 97.34% <0.00%> (+1.76%) ⬆️
.../io/smallrye/mutiny/helpers/queues/DrainUtils.java 82.22% <0.00%> (+6.66%) ⬆️
... and 1 more

@jponge
Copy link
Member

jponge commented Sep 1, 2021

Ok we can have this.

How about having DISABLE_CALLBACK_DECORATORS_PROP_NAME as a public constant?

@Sanne
Copy link
Contributor Author

Sanne commented Sep 1, 2021

Ok we can have this.

How about having DISABLE_CALLBACK_DECORATORS_PROP_NAME as a public constant?

If you prefer I can do that. But maybe it then becomes harder to remove later, assuming some better alternative arises from @FroMage 's ideas?

Personally I have no opinion, let me know if you want me to change it :)

@jponge
Copy link
Member

jponge commented Sep 1, 2021

@FroMage any better idea?

@FroMage
Copy link
Contributor

FroMage commented Sep 1, 2021

Errrr, no better idea. I guess this depends on which other system properties Mutiny uses, and whether you want to advertise this one. I was more thinking about MP Config myself, but it's more complex. I'll start a discussion on quarkus-dev about this.

@jponge
Copy link
Member

jponge commented Sep 1, 2021

On second thought I think we can keep the property private, and not advertise this too much. Thoughts @cescoffier?

@cescoffier
Copy link
Contributor

We don't have to advertise it and keep it private.

@FroMage
Copy link
Contributor

FroMage commented Sep 1, 2021

There you go, problem solved ;)

@jponge jponge added the enhancement New feature or request label Sep 1, 2021
@jponge jponge added this to the 1.1.0 milestone Sep 1, 2021
@jponge
Copy link
Member

jponge commented Sep 1, 2021

Here we go!

@jponge jponge merged commit 1c22c5e into smallrye:main Sep 1, 2021
@Sanne Sanne deleted the CPIntegration branch September 1, 2021 17:21
@Sanne
Copy link
Contributor Author

Sanne commented Sep 1, 2021

thanks all! Looking forward to use this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants