feat: Support configuring the external endpoint of Trino clusters for ackUri rewriting#100
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_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
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: