Refactor routing logics and remove unused lookups

[oneonestar]

Description

Refactor the code to separate Query ID/Cookie-based routing from new query submission routing.

Remove the unnecessary getExternalUrlForQueryId and getRoutingGroupForQueryId methods, along with their related DAOs and caches. The externalUrl and routingGroup fields are stored for newly submitted queries as before.

Additional context and related issues

getExternalUrlForQueryId and getRoutingGroupForQueryId are only used when a Query ID exists. However, their results are never persisted back to the database. We only store query history for newly submitted queries:

trino-gateway/gateway-ha/src/main/java/io/trino/gateway/proxyserver/ProxyRequestHandler.java

Lines 190 to 196 in 5bf67f3

	if (statementPaths.stream().anyMatch(request.getUri().getPath()::startsWith) && request.getMethod().equals(HttpMethod.POST)) {
	Optional<String> username = ((TrinoRequestUser) servletRequest.getAttribute(TRINO_REQUEST_USER)).getUser();
	future = future.transform(response -> recordBackendForQueryId(request, response, username, routingDestination), executor);
	if (includeClusterInfoInResponse) {
	cookieBuilder.add(new NewCookie.Builder("trinoClusterHost").value(remoteUri.getHost()).build());
	}
	}

For newly submitted queries, externalUrl and routingGroup are stored here, so this refactoring won't affect the existing behavoir:

trino-gateway/gateway-ha/src/main/java/io/trino/gateway/ha/handler/RoutingTargetHandler.java

Lines 94 to 114 in 5bf67f3

	private RoutingTargetResponse getRoutingTargetResponse(HttpServletRequest request)
	{
	RoutingSelectorResponse routingDestination = routingGroupSelector.findRoutingDestination(request);
	String user = request.getHeader(USER_HEADER);
	// This falls back on default routing group backend if there is no cluster found for the routing group.
	String routingGroup = !isNullOrEmpty(routingDestination.routingGroup())
	? routingDestination.routingGroup()
	: defaultRoutingGroup;
	ProxyBackendConfiguration backendConfiguration = routingManager.provideBackendConfiguration(routingGroup, user);
	String clusterHost = backendConfiguration.getProxyTo();
	String externalUrl = backendConfiguration.getExternalUrl();
	// Apply headers from RoutingDestination if there are any
	HttpServletRequest modifiedRequest = request;
	if (!routingDestination.externalHeaders().isEmpty()) {
	modifiedRequest = new HeaderModifyingRequestWrapper(request, routingDestination.externalHeaders());
	}
	return new RoutingTargetResponse(
	new RoutingDestination(routingGroup, clusterHost, buildUriWithNewCluster(clusterHost, request), externalUrl),
	modifiedRequest);
	}

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

[hpopuri2]

My Understanding of This PR (different cases i thought)

1. POST Request (First-time Query Execution)

• Caches backend URL only in local Caffeine cache
• Stores full query history (backend, routingGroup, externalUrl) to database as before
• No change to query history persistence

2. Follow-up Requests

• Only need backend URL to route correctly
• routingGroup and externalUrl were looked up but never actually used
• Follow-up requests don't record to query history (not POST requests)
• Safe to remove these unnecessary lookups

3. Multi-Gateway Scenario

• Gateway B retrieves backend URL only from database
• Routes to correct backend cluster
• Doesn't need routingGroup/externalUrl for routing
• Safe because routing depends solely on backend URL

4. Same proxyTo, Different Routing Groups

• Multiple routing groups can point to same backend URL
• Follow-up routing uses backend URL only, not routing group
• Safe because routing group isn't used in routing decisions

LGTM

[vishalya]

Looks like the caches - queryIdRoutingGroupCache and queryIdExternalUrlCache are safe to be removed.

[andythsu]

It looks like we used to use requestAnalyserClientsUseV2Format and requestAnalyserMaxBodySize to create TrinoQueryProprties class but since now we are storing it in request object we just never cleaned it up.

trino-gateway/gateway-ha/src/main/java/io/trino/gateway/ha/handler/ProxyUtils.java

Line 77 in abe47c2

TrinoQueryProperties trinoQueryProperties = new TrinoQueryProperties(request, requestAnalyserClientsUseV2Format, requestAnalyserMaxBodySize);

[andythsu]

The whole point of these two seemingly unnecessary cache is to not use defaultRoutingGroup because it might not be the right routing group for subsequent queries.

For example:
first query: POST/ /v1/statement goes to cluster foo_cluster in routing group foo_routing_group with cluster url https://foo-cluster
second query: GET/ ..., instead of using foo_cluster it would use whatever value is set to defaultRoutingGroup as routing group.

It may not break the existing workflow as it doesn't use routing group to route. It's routed with cluster uri, but the information being carried around with subsequent GETs is wrong.

[oneonestar]

second query: GET/ ...

I assume you're referring to the nextUri from the first request. The subsequent requests must route to the exact Trino cluster in order to process the query. Routing the subsequent requests to other Trino clusters in the same routing group doesn't make sense.

[andythsu]

yes I understand. This PR still routes the subsequent requests to the same Trino cluster as it's using clusterUri. However, routingDestination.routingGroup now points to defaultRoutingGroup instead of the actual routing group for the first POST request

For example, it would end up with
1st request: POST/ /v1/statement RoutingDestination class holds routingGroup = foo, clusterUri = foo.com
2nd request: GET/ /... RoutingDestination class holds routingGroup = <defaultRoutingGroup>, clusterUri = foo.com
it doesn't break the flow as it only uses clusterUri, but now routingGroup is wrong.

Please correct me if I'm wrong

[oneonestar]

Instead of passing an unused defaultRoutingGroup around, I updated the code to use empty string to indicate it's not used.

[oneonestar]

CI error related to #966

[andythsu]

🚀 not the best solution, but the benefits outweighs having to maintain two caches just for this info

[oneonestar]

Rebase to latest main branch.
@vishalya Would you like to take a look?

[hpopuri2]

@oneonestar can we push a ahead with this ?
so that it unblocks this PR #877

[Chaho12]

@oneonestar can you fix conflicts?

[github-actions]

This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack.