Fix eager evaluation of fallback routing in resolveRouting#922
Fix eager evaluation of fallback routing in resolveRouting#922zeckem19 wants to merge 3 commits intotrinodb:mainfrom
Conversation
Change Optional.orElse() to Optional.orElseGet() in resolveRouting to prevent eager evaluation of getRoutingTargetResponse(). With orElse(), the fallback is always evaluated even when previousCluster is present. If the fallback throws (e.g. no backends for the routing group), the exception propagates before orElse() can return the cached result. This causes 500 errors on /ui/query.html requests that have a known query ID in the cache but no X-Trino-Routing-Group header, because the routing rules cannot resolve a valid routing group for browser requests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 cla@trino.io. 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 |
|
@ebyhr i added the test, and have emailed the CLA (zeckem19) |
gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestRoutingTargetHandler.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestRoutingTargetHandler.java
Show resolved
Hide resolved
|
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 cla@trino.io. 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 |
1 similar comment
|
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 cla@trino.io. 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 |
|
Please fix CI failure: Also, please follow the PR template next time. |
Removed reference to GitHub issue 920 in test case comments. Co-authored-by: Cursor <cursoragent@cursor.com>
63f4c00 to
16a8750
Compare
|
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 cla@trino.io. 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 |
|
Thanks for fixing this. Just two cents/impact statement: In trino-gateway 16, In trino-gateway 17, Practically this means that when using |
Summary
Optional.orElse()toOptional.orElseGet()inRoutingTargetHandler.resolveRouting()to prevent eager evaluation of the fallback pathProblem
Optional.orElse(value)eagerly evaluatesvalue, inresolveRouting(), this meansgetRoutingTargetResponse(request)is always called, even when the query ID is found in the cache andpreviousClusteris present.For requests like
GET /ui/query.html?queryId(e.g. Trino Web UI tracking links via Superset), the request has no routing headers (X-Trino-Routing-Group, etc.). WhengetRoutingTargetResponse()runs the routing rules and falls through toprovideBackendConfiguration(), it throwsIllegalStateException: Number of active backends found zeroif no default backend is configured. This exception propagates before.orElse()can return the cached result, causing a 500 error even though the query ID was successfully resolved.Fix
Replace
.orElse(getRoutingTargetResponse(request))with.orElseGet(() -> getRoutingTargetResponse(request))so the fallback is only evaluated whenpreviousClusteris empty.Test plan
testResolveRoutingWithKnownQueryIdAndFailingFallbackwhich verifies that when a query ID is found in the cache but the fallback routing would throw,resolveRoutingstill returns the cached backend successfullyorElse()and passes withorElseGet()Fixes #920