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
| /// 2. In case the `external_trino_addr` is set, segments ackUri to point to the external | ||
| /// address of Trino. Trino sometimes get's confused (likely by some HTTP) headers and put's the | ||
| /// trino-lb address into the ackUri (but the requests should go to Trino directly). |
There was a problem hiding this comment.
note: Something seems to be missing here.
| /// 2. In case the `external_trino_addr` is set, segments ackUri to point to the external | |
| /// address of Trino. Trino sometimes get's confused (likely by some HTTP) headers and put's the | |
| /// trino-lb address into the ackUri (but the requests should go to Trino directly). | |
| /// 2. In case the `external_trino_addr` is set, update the segments ackUri to point to the | |
| /// external address of Trino. Trino sometimes gets confused (likely by some HTTP headers) and | |
| /// puts the trino-lb address into the ackUri (but the requests should go to Trino directly). |
| 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.
| ) -> Result<(), Error> { | ||
| // Point nextUri to trino-lb | ||
| if let Some(next_uri) = &self.next_uri { | ||
| let next_uri = Url::parse(next_uri).context(ParseNextUriFromTrinoSnafu)?; |
There was a problem hiding this comment.
note: Immediately parsing this as a URL when deserializing TrinoQueryApiResponse might be a better idea and also gets rid of the to_string call below.
| #[instrument( | ||
| fields(next_uri = %next_uri, trino_lb_addr = %trino_lb_addr), | ||
| )] | ||
| fn change_next_uri_to_trino_lb(next_uri: &Url, trino_lb_addr: &Url) -> Url { |
There was a problem hiding this comment.
note: We should not take &Url for trino_lb_addr here if the first thing we do is to clone it.
| /// 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. |
| Url::parse(&trino_external_endpoint) | ||
| .context(ParseClusterExternalEndpointFromStoredQuerySnafu)?, |
There was a problem hiding this comment.
note: Such a bummer that have to do this every time. Can we maybe natively support Url to be used as part of query!?
| /// 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.
| uri: Uri, | ||
| ) -> Result<(HeaderMap, Json<TrinoQueryApiResponse>), Error> { | ||
| ) -> Result<Response, Error> { | ||
| if method == http::Method::HEAD { |
There was a problem hiding this comment.
note: I would like to see a match here instead to explicitly handle GET and HEAD and error/warn on all other methods.
note: It might additionally also make sense to move GET and HEAD code into separate functions respectively.
| Ok((trino_headers, Json(trino_query_api_response))) | ||
| } | ||
|
|
||
| async fn handle_head_request_to_trino( |
There was a problem hiding this comment.
note: I think this function is poorly named. It implies it handles the complete request and response while it only returns a list of headers. This should be made clear.
Part of #97
Additionally, sometimes the trino-client sends a HEAD request, which we need to proxy as HEAD (ant not GET) as well: