add configurable default routing group, add indicative errors, use ex… #687

shanigeva · 2025-05-21T16:23:55Z

Improve external routing with error propagation and configurable defaults

Description

This PR is to solve request of Git Issue #658
This PR enhances Trino Gateway's external routing capabilities with three key improvements:

Error Propagation: Ensures errors from the external routing API are properly passed to clients rather than being silently ignored, addressing issue Errors from External Routing Service Are Not Propagated to Clients #658.
Configurable Default Routing: Adds the ability to configure a default routing group when the external router fails or is unavailable, improving system resilience.
Meaningful Error Messages: Enhances error reporting with detailed, structured messages that provide clear information about routing failures.

These changes improve both the reliability and usability of Trino Gateway when using external routing services.

Additional context and related issues

The main changes include:

Modified the ExternalRoutingGroupProvider to capture and propagate errors from the external API
Added configuration options for specifying a default routing group to use as fallback
Improved error message formatting to include specific details about the failure cause
Added appropriate HTTP status code mapping based on error conditions
Included request tracing information for better debugging of routing failures

Related issues:

Errors from External Routing Service Are Not Propagated to Clients #658: Errors from External Routing Service Are Not Propagated to Clients
Builds upon the external routing API functionality introduced in earlier releases

These enhancements make external routing more robust while giving administrators better visibility into routing problems, supporting more advanced multi-cluster deployment scenarios.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Improve external routing with three key enhancements:
  * Fix issue where errors from external routing services were not propagated to clients (#658)
  * Add support for configurable default routing groups when external routing fails
  * Enhance error messages with detailed information about routing failures


This revised PR description:

1. **Highlights all three improvements** you mentioned in a clear, organized way
2. **Expands the technical details** to cover the implementation of all three features
3. **Updates the release notes** to succinctly mention all three improvements
4. **Maintains the reference** to issue #658 for the error propagation component

The description now provides a complete overview of the changes while helping reviewers understand how each component fits together to improve the external routing experience in Trino Gateway.

cla-bot · 2025-05-21T16:23:57Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Chaho12 · 2025-05-21T23:37:37Z

Please resolve conflicts first :)

andythsu · 2025-05-27T22:03:44Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/RoutingManager.java

@@ -100,6 +104,10 @@ public String provideBackendForRoutingGroup(String routingGroup, String user)
    {
        List<ProxyBackendConfiguration> backends =
                gatewayBackendManager.getActiveBackends(routingGroup);
+        // Check if any backends exist for the routing group (even before filtering unhealthy ones)
+        if (backends.isEmpty()) {
+            throw new NotFoundException("Routing group does not exist: " + routingGroup);


the error message here does not reflect what this is doing. I think something like "Cannot find any backends for routing group: " + routingGroup is better

cla-bot · 2025-06-05T20:15:14Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

gateway-ha/src/main/java/io/trino/gateway/ha/config/RulesExternalConfiguration.java

RoeyoOgen · 2025-06-06T04:28:28Z

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

+                    .entity(e.getMessage())
+                    .build());
+        }
+        catch (WebApplicationException e) {


why is this 'catch' needed? you just re-throw the exact error.

RoeyoOgen · 2025-06-06T04:35:43Z

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

@@ -67,6 +71,7 @@ public RoutingTargetHandler(
    {
        this.routingManager = requireNonNull(routingManager);
        this.routingGroupSelector = requireNonNull(routingGroupSelector);
+        this.defaultRoutingGroup = haGatewayConfiguration.getRouting().getDefaultRoutingGroup();


NIT: rename getRouting() to getRoutingConfiguration()

For some reason this seems to be the convention of getters & setters for configuration objects within RoutingTargetHandler

@vishalya would love your response

I think it's ok stay with getRouting

RoeyoOgen · 2025-06-06T04:45:14Z

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

+        }
+        else {
+            routingGroup = defaultRoutingGroup;
+            clusterHost = routingManager.provideDefaultCluster(user);


why not use provideClusterForRoutingGroup?

Modified to use provideClusterForRoutingGroup, fixed fallback to default cluster within provideClusterForRoutingGroup.

RoeyoOgen · 2025-06-06T04:46:57Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/ExternalRoutingGroupSelector.java

@@ -119,6 +129,10 @@ else if (response.errors() != null && !response.errors().isEmpty()) {
            }
            return new RoutingSelectorResponse(response.routingGroup(), filteredHeaders);
        }
+        catch (WebApplicationException e) {


similar to above comment, is this catch really needed?

This catch throws the required errors from the external rest service as is (no response model building), any other errors are just logged

RoeyoOgen · 2025-06-06T05:43:09Z

CLA has been signed and sent by mail, awaits approval

cla-bot · 2025-06-06T13:45:15Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2025-06-09T14:30:08Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

EdenKik · 2025-06-09T14:36:39Z

Hey,
Updated docs & tests
PR is ready to be reviewed @Chaho12 @oneonestar @vishalya :)

docs/routing-rules.md

gateway-ha/src/main/java/io/trino/gateway/ha/config/RulesExternalConfiguration.java

cla-bot · 2025-06-10T05:28:23Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Chaho12 · 2025-06-11T01:00:44Z

@shanigeva Have you sent CLA doc?

RoeyoOgen · 2025-06-11T04:29:25Z

@shanigeva Have you sent CLA doc?

@Chaho12 , yeah CLA has been signed and sent by mail, awaits approval, maybe @mosabua can help with this

EdenKik · 2025-06-17T06:00:09Z

Rebased & squashed commits

ebyhr · 2025-06-17T06:49:42Z

gateway-ha/src/main/java/io/trino/gateway/ha/module/QueryCountBasedRouterProvider.java

 import io.trino.gateway.ha.router.QueryCountBasedRouter;
 import io.trino.gateway.ha.router.RoutingManager;

 public class QueryCountBasedRouterProvider
          extends RouterBaseModule
 {
+    private final HaGatewayConfiguration configuration;


Don't put mutable objects in fields.

ebyhr · 2025-06-17T06:49:52Z

gateway-ha/src/main/java/io/trino/gateway/ha/module/StochasticRoutingManagerProvider.java

 import io.trino.gateway.ha.router.RoutingManager;
 import io.trino.gateway.ha.router.StochasticRoutingManager;

 public class StochasticRoutingManagerProvider
            extends RouterBaseModule
 {
+    private final HaGatewayConfiguration configuration;


Don't put mutable objects in fields.

ebyhr · 2025-06-17T06:50:11Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/ExternalRoutingGroupSelector.java

@@ -67,6 +70,7 @@ public class ExternalRoutingGroupSelector
                .add("Content-Length")
                .addAll(rulesExternalConfiguration.getExcludeHeaders())
                .build();
+        this.propagateErrors = rulesExternalConfiguration.isPropagateErrors();


this. is redundant.

ebyhr · 2025-06-17T06:51:30Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/HaGatewayManager.java

    {
        dao = requireNonNull(jdbi, "jdbi is null").onDemand(GatewayBackendDao.class);
+        this.defaultRoutingGroup = requireNonNull(routingConfiguration, "routingConfiguration is null").getDefaultRoutingGroup();


We don't use requireNonNull for config classes.

ebyhr · 2025-06-17T06:52:24Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/StochasticRoutingManager.java

    public StochasticRoutingManager(
            GatewayBackendManager gatewayBackendManager, QueryHistoryManager queryHistoryManager)
    {
-        super(gatewayBackendManager, queryHistoryManager);
+        this(gatewayBackendManager, queryHistoryManager, null);


I don't think we should pass null for RoutingConfiguration.

Removed this constructor, unused

ebyhr · 2025-06-17T06:52:54Z

gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestRoutingTargetHandler.java

+        ExternalRouterResponse mockResponse = new ExternalRouterResponse(
+                "test-group", null, ImmutableMap.of());


Unwrap. Same for other test methods.

Meaning - you can have the code on a single line, the line is not long enough.

ebyhr · 2025-06-17T06:54:30Z

gateway-ha/src/test/java/io/trino/gateway/ha/router/TestRoutingManagerNotFound.java

+import static io.trino.gateway.ha.TestingJdbcConnectionManager.createTestingJdbcConnectionManager;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+@TestInstance(Lifecycle.PER_CLASS)


Do we really need this annotation?

It is required for using @BeforeAll annotation, instead of initializing properties before each test.

No need to use @BeforeAll. Please replace it with a constructor. See https://trino.io/docs/current/develop/tests.html#use-simple-resource-initialization

RoeyoOgen · 2025-06-18T07:15:02Z

@mosabua LGTM please :)

EdenKik · 2025-06-18T17:52:01Z

@vishalya
Any idea of why CI crashes? Seems like resources issue
'The forked VM terminated without properly saying goodbye. VM crash or System.exit called?'

Build passes locally

EdenKik · 2025-06-19T06:55:45Z

@vishalya
The issue was the configuration binding in some classes, derived from this comment #687 (comment)
Moved the configuration object to RouterBaseModule which makes more sense.

RoeyoOgen · 2025-06-21T20:06:36Z

@vishalya LGTM please

RoeyoOgen · 2025-06-23T08:12:35Z

@vishalya @Chaho12 @andythsu LGTM

ebyhr · 2025-06-24T06:12:16Z

docs/routing-rules.md

+route requests. If this header is not specified, requests are sent to the default routing group.
+
+You can configure a default routing group by setting the `defaultRoutingGroup` under the `routing` section. 
+This group will be used whenever routing information is unavailable or the external routing service fails. 
+If not configured, the fallback remains the built-in group `adhoc`.


Please wrap at 80 characters.

ebyhr · 2025-06-24T06:16:00Z

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

+            logRewrite(routingTargetResponse.routingDestination().clusterHost(), request);
+            return routingTargetResponse;
+        }
+        catch (NotFoundException e) {


Which code throws NotFoundException in this try block?

ebyhr · 2025-06-24T06:17:26Z

gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/GatewayBackendDao.java

+    /**
+     * @deprecated Use {@link #findActiveBackendByRoutingGroup(String)} with the configured default routing group
+     */
+    @Deprecated


Remove an unused method instead of marking as @Deprecated.

ebyhr · 2025-06-24T06:19:11Z

gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestRoutingTargetHandler.java

+                RoutingGroupSelector.byRoutingExternal(httpClient, config.getRoutingRules().getRulesExternalConfiguration(),
+                        config.getRequestAnalyzerConfig()),


ebyhr · 2025-06-24T06:20:39Z

gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestRoutingTargetHandler.java

+                RoutingGroupSelector.byRoutingExternal(httpClient, config.getRoutingRules().getRulesExternalConfiguration(),
+                        config.getRequestAnalyzerConfig()),


ebyhr · 2025-06-24T06:22:12Z

gateway-ha/src/test/java/io/trino/gateway/ha/router/TestExternalRoutingGroupSelector.java

-        assertThat(routingSelectorResponse.routingGroup()).isNull();
-        assertThat(routingSelectorResponse.externalHeaders().get(headerKey)).isNull();
+        assertThat(routingSelectorResponse.externalHeaders().get(ROUTING_GROUP_HEADER)).isNull();
+        assertThat(routingSelectorResponse.externalHeaders().get(headerKey)).isEqualTo("should-be-null");


assertThat(routingSelectorResponse.externalHeaders()) .doesNotContainKey(ROUTING_GROUP_HEADER) .containsEntry(headerKey, "should-be-null");

ebyhr · 2025-06-24T06:22:54Z

Please rebase on main to resolve conflicts.

…ternal routing url to pass errors to client Fix configurable default routing group, add indicative errors, use external routing url to pass errors to client Fix CR comments Updated docs with PR changes Added PR changes testing Added PR testing Apply suggestions from code review Co-authored-by: Jaeho Yoo <[email protected]> Fixed review comments Fixed configuration binding

Chaho12 requested review from vishalya, oneonestar and Chaho12 May 21, 2025 23:15

andythsu reviewed May 27, 2025

View reviewed changes

EdenKik force-pushed the issue-658-external-routing-errors branch from ce3c49b to 3def10a Compare June 5, 2025 20:15

RoeyoOgen reviewed Jun 6, 2025

View reviewed changes

Chaho12 reviewed Jun 10, 2025

View reviewed changes

EdenKik force-pushed the issue-658-external-routing-errors branch from ef4ecaa to 7d175a6 Compare June 17, 2025 05:34

cla-bot bot added the cla-signed label Jun 17, 2025

ebyhr reviewed Jun 17, 2025

View reviewed changes

EdenKik force-pushed the issue-658-external-routing-errors branch 4 times, most recently from f6e18b3 to 34d2ca0 Compare June 18, 2025 17:41

EdenKik force-pushed the issue-658-external-routing-errors branch from 34d2ca0 to c11934c Compare June 19, 2025 06:41

Chaho12 mentioned this pull request Jun 23, 2025

Improve backend fallback logic for unknown queryId handling #703

Open

ebyhr requested changes Jun 24, 2025

View reviewed changes

RoeyoOgen mentioned this pull request Jun 25, 2025

Add Trino Gateway 16 release notes #643

Open

EdenKik force-pushed the issue-658-external-routing-errors branch from c11934c to 99d5d71 Compare June 25, 2025 07:27

CR fix

e072b5c

EdenKik force-pushed the issue-658-external-routing-errors branch from 99d5d71 to e072b5c Compare June 25, 2025 07:32

		ExternalRouterResponse mockResponse = new ExternalRouterResponse(
		"test-group", null, ImmutableMap.of());

		RoutingGroupSelector.byRoutingExternal(httpClient, config.getRoutingRules().getRulesExternalConfiguration(),
		config.getRequestAnalyzerConfig()),

add configurable default routing group, add indicative errors, use ex… #687

Are you sure you want to change the base?

add configurable default routing group, add indicative errors, use ex… #687

Conversation

shanigeva commented May 21, 2025

Improve external routing with error propagation and configurable defaults

Description

Additional context and related issues

Release notes

Uh oh!

cla-bot bot commented May 21, 2025

Uh oh!

Chaho12 commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cla-bot bot commented Jun 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdenKik Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoeyoOgen commented Jun 6, 2025

Uh oh!

cla-bot bot commented Jun 6, 2025

Uh oh!

cla-bot bot commented Jun 9, 2025

Uh oh!

EdenKik commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cla-bot bot commented Jun 10, 2025

Uh oh!

Chaho12 commented Jun 11, 2025

Uh oh!

RoeyoOgen commented Jun 11, 2025

Uh oh!

EdenKik commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdenKik Jun 6, 2025 •

edited

Loading

EdenKik commented Jun 18, 2025 •

edited

Loading