feat: Support configuring the external endpoint of Trino clusters for ackUri rewriting #100
Conversation
|
Pushed the current version as |
|
Updated |
sbernauer
moved this from Development: In Progress
to Development: Track
in Stackable Engineering
sbernauer
moved this from Development: Track
to Development: Waiting for Review
in Stackable Engineering
|
Customer confirmed this solves their issues :) |
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
trino-lb-core/src/trino_api.rs
Outdated
| trino_lb_addr: &Url, | ||
| external_trino_addr: Option<&Url>, |
There was a problem hiding this comment.
question: Why are these parameters called *_addr if they contain URLs? I think their names should be updated to reflect this.
There was a problem hiding this comment.
Nice partial change, but I think the trino_lb_addr parameter should also be renamed.
| /// Endpoint of the Trino cluster the query is running on. | ||
| pub trino_endpoint: Url, | ||
|
|
||
| /// (Optionally, if configured) public endpoint of the Trino cluster. |
There was a problem hiding this comment.
suggestion: Slight reword. The fact that this option is optional implies that it only takes effect when configured.
| /// (Optionally, if configured) public endpoint of the Trino cluster. | |
| /// Optional public endpoint of the Trino cluster. |
There was a problem hiding this comment.
I prefer my variant as it makes it a bit more clear.
Every Trino always has a public endpoint. The question is, if trino-lb was configured to know about it or not
There was a problem hiding this comment.
Maybe Optionally use the ... is better then?
| /// special handling. | ||
| #[instrument( | ||
| name = "GET /v1/statement/executing/{queryId}/{slug}/{token}", | ||
| name = "GET (or HEAD) /v1/statement/executing/{queryId}/{slug}/{token}", |
There was a problem hiding this comment.
note: Ideally we set this to the appropriate HTTP method in the function itself, but I'm unsure if this is supported (without using the special otel.name field/attribute).
| name = "GET (or HEAD) /v1/statement/executing/{queryId}/{slug}/{token}", | ||
| skip(state, headers) | ||
| )] | ||
| pub async fn get_trino_executing_statement( |
There was a problem hiding this comment.
note: This function needs to be renamed because it now now only handles GET requests, but HEAD as well.
There was a problem hiding this comment.
That was the very surprising implementation details of axum, which caused the "bug" in the first place.
Even if you register it via get(), is still get's HEADs.
We register the fn with
.route(
"/v1/statement/executing/{query_id}/{slug}/{token}",
get(v1::statement::get_trino_executing_statement),
)We do this many times with lot's of functions, so we would need to change all of them.
And I currently find the registration code readable
There was a problem hiding this comment.
That was the very surprising implementation details of axum, which caused the "bug" in the first place.
That's true, is there any documentation on this behaviour?
I'm fine with only renaming this function, because we now explicitly handle both HTTP methods.
Co-authored-by: Techassi <git@techassi.dev>
2 participants
Part of #97
Additionally, sometimes the trino-client sends a HEAD request, which we need to proxy as HEAD (ant not GET) as well: