router_check: add timeout route assertion to router_check_tool#46019
Open
varungandhi-databricks wants to merge 1 commit into
Open
Conversation
varungandhi-databricks
had a problem deploying
to
external-contributors
GitHub Actions
Error
|
Hi @varungandhi-databricks, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
varungandhi-databricks
force-pushed
the
varungandhi-databricks/router-check-timeout-assertion
branch
from
cb8e189 to
606eda5
Compare
varungandhi-databricks
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
varungandhi-databricks
force-pushed
the
varungandhi-databricks/router-check-timeout-assertion
branch
from
606eda5 to
3555c91
Compare
varungandhi-databricks
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
agrawroh
reviewed
Jul 9, 2026
| exit 1 | ||
| fi | ||
|
|
||
| # Timeout assertion failure output (route selected correctly by cluster, but wrong timeout) |
Member
There was a problem hiding this comment.
Please rephrase this. It can never happen that you "select" a correct route but then we apply a different timeout. You can give an example of RegEx route with a fallback where the fallback with a different timeout gets picked in case nothing RegEx is not matched.
There was a problem hiding this comment.
Fixed this. The test now asserts a more realistic case similar to the one I had in mind earlier with regexes. PTAL.
varungandhi-databricks
force-pushed
the
varungandhi-databricks/router-check-timeout-assertion
branch
from
3555c91 to
c88238e
Compare
varungandhi-databricks
had a problem deploying
to
external-contributors
GitHub Actions
Error
Add a `timeout` field to ValidationAssert so tests can assert the matched route's per-route request timeout (RouteEntry::timeout()), not just the selected cluster. compareTimeout reads routeEntry()->timeout() and compares in milliseconds. Useful when two routes share the same upstream cluster but carry different per-route timeouts -- cluster_name alone cannot distinguish which was selected. Adds Timeout.yaml + golden fixtures and a TimeoutFailures fixture asserting the mismatch error string via route_tests.sh. Signed-off-by: Varun Gandhi <varun.gandhi@databricks.com>
varungandhi-databricks
force-pushed
the
varungandhi-databricks/router-check-timeout-assertion
branch
from
c88238e to
a273935
Compare
varungandhi-databricks
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The router_check_tool can currently assert the selected cluster, virtual host, path rewrite, and header fields -- but not the matched route's per-route timeout. This means tests cannot distinguish two routes that share the same upstream cluster but carry different timeouts.
This PR adds a
timeoutfield toValidationAssert.compareTimeoutreadsrouteEntry()->timeout()and compares in milliseconds, following the same pattern ascompareCluster.Commit Message: router_check: add timeout route assertion to router_check_tool
Additional Description: Adds
google.protobuf.Duration timeouttoValidationAssert(field 14) and the correspondingexpected_timeout/actual_timeoutfields toValidationFailure.compareTimeoutis registered in thecheckers[]array and returns true whenhas_timeout()is false, so existing tests are unaffected.Risk Level: Low
Testing: Added
Timeout.yaml+ golden fixtures (two routes to the same cluster with different per-route timeouts, matched by prefix and regex) and aTimeoutFailuresfixture asserting the mismatch error string viaroute_tests.sh.Docs Changes: N/A
Release Notes: N/A