-
Notifications
You must be signed in to change notification settings - Fork 31
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
chore(vertx): upgrade vertx 3->4 #949
Conversation
Would it be a substantial change to use vertx 4.2.5 instead of 4.3? This would allow us to sync with Quarkus 2.7: https://github.com/quarkusio/quarkus/blob/2733bae7cb7ded76d324b41a3a9f4e81b89ee122/bom/application/pom.xml#L114 |
4.2.5 would probably work with no changes. The only difference I can anticipate is that the https://github.com/vert-x3/wiki/wiki/4.3.0-Release-Notes I can try a 4.2.5 build and see what breaks, if anything. |
@ebaron latest push is downgrade to vertx 4.2.5 now. The GraphQL stuff was actually all fine, the only required changes were to go back to the |
src/main/java/io/cryostat/recordings/RecordingTargetHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good. Tested it out and Rules
are working well but Recording Stopped
notifications aren't being issued (see my comment above).
Fixes #453
This is a very large looking PR, but it only does a couple of things. They just touched a lot of places.
First it upgrades the Vert.x version in the
pom.xml
from a 3.x to 4.3.0 (latest). This comes with a change moving theHttpStatusException
class to a different package and renaming itHttpException
, as well as making it final. This is the bulk of the file touches in the PR, because we were usingHttpStatusException
nearly everywhere. It moved from Vert.x "internal" package to a public API one at Vert.x version 4.1.0, so this shouldn't happen to us again. This necessitated a small adjustment to theApiException
handling in various places as well, since that can no longer subclass thefinal class HttpException
.Next it makes a few small adjustments to get functionality working again due to various changes in the 4.x line. For example, the Vert.x-GraphQL
GraphQLHandler
is no longer aBodyHandler
on its own, so I had to add a specificBodyHandler
instance in front of that one like we have elsewhere. The GraphQL Scalar type was also deprecated, removed, and replaced by one in theextended-scalars
package.Next it turns a few more pieces into Verticles. For the most part this isn't actually achieving anything at this point, since these new pieces aren't taking much advantage of Vert.x event loop architecture, but this opens the door to refactor them better in the future.
Finally it replaces various
(Scheduled)ExecutorService
usages withvertx.setTimer()
andvertx.setPeriodic
. This lets us make better use of the existing Vert.x worker thread pool, rather than maintaining our own competing thread pool(s). Unlikely to have serious impact here yet, but it allows us to refactor things to make better use of the event loop and reactive/non-blocking architecture in the future.