Refactor http servers #554
Conversation
mikkokar
force-pushed
the
refactor-http-servers
branch
from
3ca6891 to
323340e
Compare
(Remove a separate connector).
Run phase1 startup actions in a separate thread.
mikkokar
force-pushed
the
refactor-http-servers
branch
from
323340e to
c37525e
Compare
| } else { | ||
| LOGGER.info("Starting {} failed", name); |
There was a problem hiding this comment.
do we need a log line next to an exception?
There was a problem hiding this comment.
Well not necessarily. But let me add the service name to the exception message.
| } else { | ||
| LOGGER.info("Stoppig {} failed", name); |
There was a problem hiding this comment.
Same here. Maybe we can unify the stopping failed message with the exception.
| components.nettySocketChannelClass()) | ||
| .create(); | ||
|
|
||
| // Startup phase 1: start plugins, control plane providers, and other services: |
There was a problem hiding this comment.
Can we break up this method?
There was a problem hiding this comment.
Not really, because this is in a constructor, and assigning to final variables.
But I will be returning to this soon. Expect more changes, but in separate increments.
| } | ||
|
|
||
| private LiveHttpResponse.Transformer addInfoHeader(LiveHttpResponse.Transformer responseBuilder, LiveHttpRequest request) { | ||
| return responseBuilder.header(styxInfoHeaderName, responseInfoFormat.format(request)); | ||
| } | ||
|
|
||
| public ProxyServerBuilder handlerFactory(Supplier<HttpHandler> handlerFactory) { | ||
| this.handlerFactory = handlerFactory; | ||
| public ProxyServerBuilder handler(HttpHandler handlerFactory) { |
There was a problem hiding this comment.
can we rename handlerFactory if it is not a factory anymore?
| try { | ||
| return new InetSocketAddress(host, it.port()); | ||
| } catch (IllegalArgumentException e) { | ||
| return null; |
There was a problem hiding this comment.
Can we at least log anything here?
There was a problem hiding this comment.
No.
This is not necessarily an erroneous situation. NettyServer is a stateful object. Returning null implies the server is shut (or not started). Whether or not this is acceptable depends on caller's use case. Logging a message therefore needs to take place at call site.
| CompletableFuture.runAsync(() -> { | ||
| // doStart should return quicly. Therefore offload waiting on a separate thread: | ||
| this.phase1Services.addListener(new Phase1ServerStartListener(this)); | ||
| this.phase1Services.startAsync().awaitHealthy(); | ||
|
|
||
| this.phase2Services.addListener(new ServerStartListener(this)); | ||
| this.phase2Services.startAsync(); | ||
| }); |
There was a problem hiding this comment.
If anything in here fails, is the exception being logged?
There was a problem hiding this comment.
I have made some changes to log them from StyxServer logger.
Sadly the shutdownLogging mechanism (see in StyxServer.java) prevents the tests from working correctly. I need to look into removing this in near future.
Remove shutdownLogging mechanism from styx core.
Need to investigate later.
mikkokar
force-pushed
the
refactor-http-servers
branch
from
54a59db to
a38330b
Compare
Rationale
Simplify Styx HTTP server implementation: