Make sure that management router is reloaded #41353
Conversation
| @@ -567,7 +581,8 @@ public void handle(RoutingContext event) { | |||
|
|
|||
| event.select(ManagementInterface.class).fire(new ManagementInterfaceImpl(managementRouter.getValue())); | |||
|
|
|||
| VertxHttpRecorder.managementRouter = handler; | |||
| VertxHttpRecorder.managementRouterDelegate = handler; | |||
| VertxHttpRecorder.managementRouter = e -> VertxHttpRecorder.managementRouterDelegate.handle(e); | |||
There was a problem hiding this comment.
Probably better avoid the lambda here.
|
Hey Clement 😃 👋🏻, Thanks for taking a look at this one.
I've been testing it on the search.quarkus.io app and the change helped there, both with adding/removing management endpoints and with modifying the handler. I found this test for live-reload https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/devmode/LiveReloadArtifactTest.java, so will try to add some tests using a similar approach to cover some of the management endpoint reload scenarios. |
| @Override | ||
| public void handle(RoutingContext event) { | ||
| Thread.currentThread().setContextClassLoader(currentCl); | ||
| hotReplacementHandler.handle(event); |
There was a problem hiding this comment.
I'm surprised we don't restore the previous CL, here?
There was a problem hiding this comment.
I can only assume that it's probably so that all subsequent handlers would run with this currentCl and it could be restored in the very last handler, but there's probably not much benefit in that?
|
+1 for having a dedicated test. |
marko-bekhta
force-pushed
the
fix/i41315-management-handler-reload
branch
2 times, most recently
from
7734ec8
to
2e46aa0
Compare
... and decide in the recorder whether we will actually keep that management router or not.
marko-bekhta
force-pushed
the
fix/i41315-management-handler-reload
branch
from
2e46aa0
to
a9f6135
Compare
|
I've rebased the PR and marked it as ready-for-review since ... you've been reviewing it 😃 |
fixes #41315
I'm opening this as a draft only, as that's probably a bad idea for the fix 🙈 😕 but then ... it helps.