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.
There was a problem hiding this comment.
I intentionally did not update that one, as that is how it is called in the config. Long term we could rename it everywhere, including in the config, but that would be a breaking change :/
There was a problem hiding this comment.
Alright, let's keep it as is, but should we create an issue for that then?
There was a problem hiding this comment.
It's actually a bigger story and the current variable naming correct: #105
trino-lb-core/src/trino_query.rs
Outdated
| /// 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?
There was a problem hiding this comment.
I think the relevant part is to mention that this depends on the trino-lb config.
WDYT of this?
/// Public endpoint of the Trino cluster, if it's configured in the trino-lb config.
/// This can e.g. be used to change segment ackUris to.
| /// 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).
There was a problem hiding this comment.
Do you plan to do any work on this (if it is indeed possible)?
There was a problem hiding this comment.
I'm personally fine as is
| 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.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we should add a dev comment either here or at the caller site refer to that piece of information from the axum docs.
Co-authored-by: Techassi <[email protected]>
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: