Show RoutingGroup info in query history #607
Conversation
andythsu
force-pushed
the
routing_group
branch
4 times, most recently
from
f57f2ad to
d23d058
Compare
andythsu
force-pushed
the
routing_group
branch
from
d23d058 to
ce90b2f
Compare
| { | ||
| Optional<String> previousBackend = getPreviousBackend(request); | ||
| String clusterHost = previousBackend.orElseGet(() -> getBackendFromRoutingGroup(request)); | ||
| // This falls back on adhoc routing group if no routing group can be determined | ||
| String routingGroup = routingGroupSelector.findRoutingGroup(request).orElse("adhoc"); |
There was a problem hiding this comment.
moving this call out of the orElseGet will cause the routing logic to be triggered for every incoming request, including those containing query IDs. Please revert
| */ | ||
| package io.trino.gateway.ha.handler; | ||
|
|
||
| public record RoutingDestinationInfo(String group, String clusterUri) {} |
There was a problem hiding this comment.
It would expand the scope of this change, but it seems like a good time to add some audibility on which rules were triggered. For example, you could have RoutingDestinationInfo(String group, String clusterUri, List<String> rulesFired), where rulesFired would contain the names of all rules who's condition evaluated to true. We can chat about this offline
| */ | ||
| package io.trino.gateway.ha.handler; | ||
|
|
||
| public record RoutingDestinationInfo(String group, String clusterUri) {} |
There was a problem hiding this comment.
Can clusterUri be URIinstead of aString`?
| @@ -181,6 +183,8 @@ private void performRequest( | |||
| ImmutableList.Builder<NewCookie> cookieBuilder = ImmutableList.builder(); | |||
| cookieBuilder.addAll(getOAuth2GatewayCookie(remoteUri, servletRequest)); | |||
|
|
|||
| requestBuilder.addHeader("X-Routing-Group", routingDestinationInfo.group()); | |||
There was a problem hiding this comment.
Can you pass the routing info directly to recordBackendForQueryId in the future.transform call? If not, then use ROUTING_GROUP_HEADER from RoutingGroupSelector as the header key
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE query_history | |||
| ADD routing_group VARCHAR(255); | |||
There was a problem hiding this comment.
nit: end the file with a newline. This goes for the other .sql files as well
| @@ -85,11 +85,11 @@ void testByRoutingGroupHeader() | |||
| // If the header is present the routing group is the value of that header. | |||
| when(mockRequest.getHeader(ROUTING_GROUP_HEADER)).thenReturn("batch_backend"); | |||
| assertThat(RoutingGroupSelector.byRoutingGroupHeader().findRoutingGroup(mockRequest)) | |||
| .isEqualTo("batch_backend"); | |||
| .contains("batch_backend"); | |||
There was a problem hiding this comment.
why do we need to change this?
| String clusterHost = previousBackend.orElseGet(() -> getBackendFromRoutingGroup(request)); | ||
| // This falls back on adhoc routing group if no routing group can be determined | ||
| String routingGroup = routingGroupSelector.findRoutingGroup(request).orElse("adhoc"); | ||
| String user = request.getHeader(USER_HEADER); |
There was a problem hiding this comment.
This may not be always true, especially in case of bearer tokens (JWT). But I see that this logic is from before and it's not newly added as a part of the PR.
| */ | ||
| package io.trino.gateway.ha.handler; | ||
|
|
||
| public record RoutingDestinationInfo(String group, String clusterUri) {} |
There was a problem hiding this comment.
name RoutingDestination would be better.
Description
It is useful to show what routing group a request is being routed to because if two routing groups have the same
routedTourl, it will show the wrong information on the history pageOld:
New:
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(X) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: