Replace Dropwizard with Airlift #317
Conversation
gateway-ha/etc/config.properties
Outdated
| node.id=ffffffff-ffff-ffff-ffff-ffffffffffff | ||
| node.environment=test |
There was a problem hiding this comment.
The gateway is not clustered, why does it need a node.id and node.environment?
There was a problem hiding this comment.
That's because HttpServerModule depends on NodeInfo which needs NodeConfig which requires non-null node.environment and node.id.
I should be able to set these in Java and eliminate these from the config file.
Might need to dig deeper to see if there are drawbacks of doing it.
There was a problem hiding this comment.
There is no harm in having these value though .. right?
oneonestar
force-pushed
the
star/airlift_try3
branch
4 times, most recently
from
349c6e1
to
4c3f20a
Compare
gateway-ha/src/main/java/io/trino/gateway/ha/config/HaGatewayConfiguration.java
Outdated
Show resolved
Hide resolved
oneonestar
force-pushed
the
star/airlift_try3
branch
14 times, most recently
from
ab38025
to
d2a698e
Compare
oneonestar
changed the title
|
I assume this is ready for PR right? |
| @@ -32,6 +26,3 @@ modules: | |||
| managedApps: | |||
| - io.trino.gateway.ha.GatewayManagedApp | |||
| - io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor | |||
|
|
|||
| logging: | |||
| type: external | |||
There was a problem hiding this comment.
Can you update installation doc too?
https://github.com/trinodb/trino-gateway/blob/main/docs/installation.md?plain=1#L103-L112
There was a problem hiding this comment.
Question. How is this different to httpConfig properties that you can set with https://trino.io/docs/current/admin/properties-logging.html properties using -Dconfig=var/config.properties?
There was a problem hiding this comment.
httpConfig works the same as setting -Dconfig. There was a discussion on Slack about the pros and cons of moving from YAML to properties, mixing YAML and properties, and keeping everything in YAML. We agreed to keep everything in YAML for now.
Since we chose to use httpConfig, -Dconfig won't work. They are mutually exclusive: https://github.com/airlift/airlift/blob/538e1ff498bca145c5ec4d582692367694a94fc8/bootstrap/src/main/java/io/airlift/bootstrap/Bootstrap.java#L151-L156
Yes. the all-or-nothing changes are done in this PR. Reviews are welcome. |
oneonestar
force-pushed
the
star/airlift_try3
branch
2 times, most recently
from
e1a0234
to
1d6fab8
Compare
oneonestar
force-pushed
the
star/airlift_try3
branch
2 times, most recently
from
d0b235d
to
ca2e9c4
Compare
oneonestar
force-pushed
the
star/airlift_try3
branch
from
ca2e9c4
to
ca04a23
Compare
| @Override | ||
| public void log(Request request, Response response) | ||
| { | ||
| // Logging without filter as both request and response don't contain sensitive information |
There was a problem hiding this comment.
We should document this change for migration, as its used for debugging. I think the information is redundant with that in QueryIdCachingProxyHandler.logRewrite
There was a problem hiding this comment.
|
|
||
| import static jakarta.ws.rs.core.Response.Status.NOT_FOUND; | ||
|
|
||
| @Path("/") |
There was a problem hiding this comment.
this will prevent forwarding / to a backend correct? In some cases I forward /, and access the TGW UI on the app port. IMO we should put all the UI assets behind a prefix such as /tgw, and make the prefix configurable if possible
Description
Resolve #41
Commits cannot be built and tested individually. This is caused by the mass dependency conflict between Dropwizard and Airlift. It is impossible to replace Dropwizard part by part and keep each commit buildable. Also, it will be very difficult to review if all changes are squashed into one commit.
TODO in this PR
Follow-up later in other PRs
How to run in Intellij
VM options:
-Xmx1g -Djdk.attach.allowAttachSelf=trueMain class:
io.trino.gateway.ha.HaGatewayLauncherArgument:
docs/quickstart-config.yamlWorking directory:
$ProjectFileDir$Release notes
(x) Release notes are required, with the following suggested text: