Migrate Jetty Proxy to Airlift

oneonestar commented 3 weeks ago

Description

Second part of Airlift migration #41. Depends on #317. This PR migrate Jetty Proxy (ie. ProxyHandler and ProxyServletImpl) to Airlift.

Motivation

Fix #242. Merge requestRouter, applicationConnectors and adminConnectors ports into one single port managed by Airlift. The TLS issues caused by routing between these ports will be gone.
Better control of the proxy request and response logic. The old implementation used Jetty ProxyServlet and overwrote different methods, making it difficult to add new features. By handling the request and response manually, we have full control of the request/response flow. This makes implementing query retries or query result caching much easier.

All configurations related to networking and TLS are done by Airlift config, which aligns with Trino. For example, the following config enable TLS for gateway:

httpConfig:
http-server.http.enabled: false
http-server.https.enabled: true
http-server.https.port: 9080
http-server.https.keystore.path: cert.jks
http-server.https.keystore.key: changeme
node.environment: test

TODO

[x] Update docs on how to config & run
[x] Document the incompatible changes and provide a migration guide.

Release notes

( ) Release notes are required, with the following suggested text:

* Replace Jetty Proxy with Airlift
* (TODO) Document the incompatible changes

mosabua commented 1 week ago

I merged #317 now. Once you rebase here you might be able to address my comment about the config here. I think we should end up with the same config as in Trino ideally. So

jvm.config config.properties node.properties (maybe not even necessary) log.properties

If we leave the http config in this PR in the YAML file for now I am good with that but maybe we can already have all of them under one config node or so

oneonestar commented 1 week ago

jvm.config
- This is read by launcher.py. We can't set JVM config the config file unless we write our own version of launcher.py which supports YAML.
node.properties (maybe not even necessary)
- Similar to above, launcher.py read this file and prepare the directories and symlinks. We can set log.output-file in httpConfig without using node.properties.
log.properties
- Airlift expects this as a file LoggingConfiguration.java. We can allow setting log level in YAML, read the YAML, write them to a properties file and sent the file path to Airlift. I'm not sure if we want to do this.
config.properties
- Everything that can be set in config.properties for Airlift can be set in httpConfig.

mosabua commented 1 week ago

jvm.config

This is read by launcher.py. We can't set JVM config the config file unless we write our own version of launcher.py which supports YAML.

Yeah.. we can do that later .. ideally NOT with a python requirement.

node.properties (maybe not even necessary)

Similar to above, launcher.py read this file and prepare the directories and symlinks. We can set log.output-file in httpConfig without using node.properties.

log.properties

Agreed

Airlift expects this as a file LoggingConfiguration.java. We can allow setting log level in YAML, read the YAML, write them to a properties file and sent the file path to Airlift. I'm not sure if we want to do this.

Probably not in this PR. We will work with the airlift team to figure out how to proceed in terms of supporting yaml or toml or whatever to do next.

config.properties

Everything that can be set in config.properties for Airlift can be set in httpConfig.

Given the scope is way larger than just configuring HTTP I would hope we can come up with a better name.. just config or globalConfig or I dont know..

oneonestar commented 1 week ago

How about serverConfig?

mosabua commented 1 week ago

How about serverConfig?

Thats good with me. @vishalya @willmostly @Chaho12 ?

Chaho12 commented 1 week ago

How about serverConfig?

sounds good as it replaces requestRouter & server configs in yaml.

Btw why is historySize and requestBufferSize removed? does gateway now read all data from history?

oneonestar commented 1 week ago

For historySize, AFAIK we never use this value. No code is using this value since the initial git commit.

The equivalent for requestBufferSize is http-client.request-buffer-size in Airlift. Let me update the doc for that.

oneonestar commented 1 week ago

Update the docs for requestBufferSize
Rename httpConfig to serverConfig PTAL

Chaho12 commented 1 week ago

For historySize, AFAIK we never use this value. No code is using this value since the initial git commit.

Yeah code does not use historySize at all but i think the intent was to limit number of history in one api call (api requests all history at once so if too many history is called at once, ui lags) i don't think this is needed anymore in current ui. Pagination is added.

mosabua commented 4 days ago

Could you maybe review @wendigo since you know a lot about this stuff already?

wendigo commented 2 days ago

@mosabua Let's merge it and improve in follow ups

mosabua commented 2 days ago

We decided in team sync today that we will merge early next week to give @vishalya and others a chance to look.

wendigo commented 2 days ago

@mosabua sure, I'd prefer to merge it now and improve it right away but I'm fine with delaying that

mosabua commented 2 days ago

@mosabua sure, I'd prefer to merge it now and improve it right away but I'm fine with delaying that

Monday ;-)

wendigo commented 2 days ago

@mosabua ages :P

trinodb / trino-gateway