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

[webflux] Re-enable blockhound and whitelist BoundedElasticScheduler.schedule #12736

Merged
merged 2 commits into from
Nov 11, 2020

Conversation

murdos
Copy link
Contributor

@murdos murdos commented Oct 13, 2020

Fixes #11599


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@murdos murdos changed the title Re-enable blockhound and whitelist BoundedElasticScheduler.schedule [webflux] Re-enable blockhound and whitelist BoundedElasticScheduler.schedule Oct 13, 2020
@murdos murdos requested a review from cbornet October 13, 2020 20:05
@murdos murdos added the theme: reactive ⚛️ Spring WebFlux label Oct 14, 2020
@@ -0,0 +1 @@
<%= packageName %>.config.JHipsterBlockHoundIntegration
Copy link
Member

@cbornet cbornet Oct 16, 2020

Choose a reason for hiding this comment

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

Maybe use @AutoService instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure adding yet another dependency and another annotation processor is worth the value here, for something that is supposed to be a temporary workaround.

Copy link
Member

Choose a reason for hiding this comment

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

Adding a file that will need to be cleaned up later doesn't seem better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does really cleaning one file (JHipsterBlockHoundIntegration.java) or two files (JHipsterBlockHoundIntegration.java + meta-inf service declaration) makes a difference?

On the other hand, we have to deal with adding a new dependency (com.google.auto.service:auto-service-annotations), a new annotation processor (com.google.auto.service:auto-service) and a new property for the version that will have to be updated from time to time, for 2 build systems (gradle and maven).

I'm really not convinced here, in this context, with the use of @Autoservice, but I can add it others think it's the better solution.
@jhipster/developers what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests pass - ship it! 🚢

@cbornet
Copy link
Member

cbornet commented Nov 11, 2020

LGTM. Feel free to merge @murdos

@murdos murdos merged commit d25bb0a into jhipster:main Nov 11, 2020
@murdos murdos deleted the enable-blockhound branch November 11, 2020 11:16
@mshima
Copy link
Member

mshima commented Nov 11, 2020

@murdos webflux started failing with this PR.

Suddenly webflux tests started failing at #13021 with a minor change and I had now clue why.
MariaDB just timeout without no useful error.
Changing sqfull from MariaDB to postgres shows the error.

Merged #13031 to confirm.

@murdos
Copy link
Contributor Author

murdos commented Dec 23, 2021

@pascalgrimaud
Copy link
Member

@murdos : approved

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

Successfully merging this pull request may close these issues.

Re-enable BlockHound once blocking calls are fixed
5 participants