Avoid null cache writes for query routing mappings by panjwanirahul · Pull Request #963 · trinodb/trino-gateway

panjwanirahul · 2026-03-15T08:47:51Z

Guard query-id cache writes in routing manager against null/empty values to prevent runtime failures when query history rows have been removed by retention.

What changed

Updated BaseRoutingManager:
- setBackendForQueryId(...): skip empty queryId; invalidate entry if backend is null/empty.
- setRoutingGroupForQueryId(...): skip empty queryId; invalidate entry if routing group is null/empty.
- setExternalUrlForQueryId(...): skip empty queryId; invalidate entry if externalUrl is null.
Added regression tests in TestRoutingManagerExternalUrlCache:
- testNullExternalUrlDoesNotThrowAndFallsBackToQueryHistory
- testNullQueryIdDoesNotThrowForExternalUrlCacheSet

Why

When query history retention deletes older records, lookup for externalUrl can return null.
The previous code attempted to cache that null value, which caused an NPE in the cache path.
The new behavior treats null/empty mappings as “not cacheable” and preserves normal fallback behavior.

Additional context and related issues

Related issue: Unhandled NullPointerException if request is for an unknown query ID #787
Behavior impact:
- Fixes NPE in query routing cache update path.
- Does not change successful routing behavior for valid mappings.
- For missing mappings, cache entry is invalidated/skipped and existing fallback logic continues.
Local validation:
- TestRoutingManagerExternalUrlCache passed.
- Routing-focused suite passed (TestRoutingManagerExternalUrlCache, TestRoutingGroupSelector, TestRoutingRulesManager, TestPathFilter, TestTrinoRequestUser, TestTrinoQueryProperties).

Release notes

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

* Fix a NullPointerException in query routing cache updates when retained query history entries are removed and external URL lookup returns null.

cla-bot · 2026-03-15T08:47:54Z

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

rahulpanjwani17 · 2026-03-30T09:22:19Z

@ebyhr Hey, can you please check this pull request and also re run the cla action? i just got the mail that my cla is signed
Thanks!!

ebyhr · 2026-03-30T09:51:44Z

@cla-bot check

cla-bot · 2026-03-30T09:51:49Z

The cla-bot has been summoned, and re-checked this pull request!

rahulpanjwani17 · 2026-03-30T10:45:41Z

@ebyhr thank you so much for re running the cla bot, can you also review this pull request?

panjwanirahul · 2026-04-02T18:50:45Z

@ebyhr Hey!! Please get a review on this? It would be great if you could please check this.

panjwanirahul · 2026-04-03T05:57:14Z

@mosabua hello, can i please get a review on this? Pending since a long time, thank you so much!

ebyhr · 2026-04-03T06:00:25Z

    @Override
    public void setBackendForQueryId(String queryId, String backend)
    {
+        if (Strings.isNullOrEmpty(queryId)) {


Let's import isNullOrEmpty directly.

ebyhr · 2026-04-03T06:01:09Z

    public void setBackendForQueryId(String queryId, String backend)
    {
+        if (Strings.isNullOrEmpty(queryId)) {
+            log.warn("Skipping backend cache update for empty queryId");


Why is the log level warning? What is the expected action when Trino Gateway admin sees this message?

Guard query-id cache writes in routing manager against null/empty values to prevent runtime failures when query history rows have been removed by retention.

The PR description indicates this is a normal scenario. The log level should be debug if so.

makes sense, thanks for the review, made the changes @ebyhr

cla-bot · 2026-04-13T18:58:46Z

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

panjwanirahul · 2026-04-15T09:35:02Z

@ebyhr can you please check this again?

panjwanirahul · 2026-04-16T15:57:16Z

@mosabua can i please get a review here? Thanks much

panjwanirahul · 2026-04-19T18:49:59Z

@mosabua @ebyhr can you folks please check this or assign someone? Thanks much!

vishalya

My understanding of the flow is this: The trino-gateway first checks its in-memory cache for the query, then falls back to the query history. But if both caches have expired or don't contain the query id, the router would have to reach out directly to the actual clusters to discover where that query is currently executing. So I'm puzzled—how do we actually end up in a situation where both caches miss?
Could you explain why deleting the history would stop the search for the actual backend?

panjwanirahul · 2026-04-22T20:47:57Z

My understanding of the flow is this: The trino-gateway first checks its in-memory cache for the query, then falls back to the query history. But if both caches have expired or don't contain the query id, the router would have to reach out directly to the actual clusters to discover where that query is currently executing. So I'm puzzled—how do we actually end up in a situation where both caches miss? Could you explain why deleting the history would stop the search for the actual backend?

@vishalya
The discovery logic is actually a bit separate from this fix. This is really just a safety fix for an NPE—caches crash if you try to .put() a null value, which is what happens once history is purged by retention. This PR just adds a guard so the gateway doesn't crash when it hits a null mapping, allowing the normal fallbacks to continue.

vishalya · 2026-04-24T15:17:45Z

I’m trying to trace the crash path and figure out whether we’re masking the real issue somewhere along the way.

ebyhr

Please squash commits into one.

panjwanirahul · 2026-05-05T19:01:05Z

@ebyhr done, please check now

panjwanirahul · 2026-05-11T17:11:25Z

@ebyhr @mosabua please check this

Chaho12 · 2026-05-13T13:31:25Z

@panjwanirahul Can you tell us how we can reproduce the situation? Steps that makes this NPE happen?

github-actions · 2026-06-03T18:26:55Z

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

github-actions · 2026-06-25T17:51:01Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

cla-bot · 2026-08-14T11:14:51Z

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

panjwanirahul · 2026-08-14T11:14:51Z

@Chaho12 Thanks for asking — here are reproducible steps. I also rebased this PR onto current main; the issue is still present.

Why this happens

Caffeine (and previously Guava) does not allow null cache values. When query-history retention has deleted the row, lookups can return null. Writing that null into the query-id caches throws NPE.

To @vishalya’s earlier question: backend lookup has an extra fallback (searchAllBackendForQuery probes clusters). Routing-group / external-url lookup do not — they only consult query history. So a history miss returns null; attempting to cache that null is what crashes. This PR does not change discovery behavior; it only prevents caching null/empty mappings so the request path can continue with the existing fallbacks (defaultRoutingGroup / cluster host).

Minimal unit-test reproduction (current main)

routingManager.setRoutingGroupForQueryId("20260315_query", null);
// throws NullPointerException from Caffeine put

routingManager.setExternalUrlForQueryId("20260315_query", null);
// throws NullPointerException from Caffeine put

On this branch those calls no longer throw; the cache entry is invalidated/skipped and later find* falls back to query history as usual.

End-to-end reproduction

Run gateway with query history enabled and a short retention window (or manually delete the history row).
Submit a statement so a queryId is recorded in history + in-memory cache.
Delete the history row for that queryId (or wait for retention), and clear/expire the in-memory cache (restart gateway, or wait for the 30-minute access expiry).
Send a follow-up request that includes that queryId (e.g. statement status / next URI), or any path that calls set*ForQueryId(..., null) after a history miss.
On unfixed main: NPE from cache put.
With this PR: no NPE; routing continues via existing fallbacks.

Local validation on latest main

Unfixed main: null setRoutingGroupForQueryId / setExternalUrlForQueryId throw NPE.
This branch: TestRoutingManagerExternalUrlCache passes, including null/empty external URL guards.

Happy to address any further review comments.

Guard query-id cache writes in routing manager against null/empty values to prevent runtime failures when query history rows have been removed by retention.

panjwanirahul closed this Mar 30, 2026

panjwanirahul reopened this Mar 30, 2026

cla-bot Bot added the cla-signed label Mar 30, 2026

kbhatianr approved these changes Mar 31, 2026

View reviewed changes

ebyhr reviewed Apr 3, 2026

View reviewed changes

cla-bot Bot removed the cla-signed label Apr 13, 2026

panjwanirahul force-pushed the fix-787-null-externalurl-cache-npe branch from 3bfc2be to 3ad9da9 Compare April 13, 2026 19:01

cla-bot Bot added the cla-signed label Apr 13, 2026

vishalya reviewed Apr 21, 2026

View reviewed changes

ebyhr reviewed May 4, 2026

View reviewed changes

panjwanirahul force-pushed the fix-787-null-externalurl-cache-npe branch from 7ff4fa9 to 3a874a9 Compare May 5, 2026 18:59

github-actions Bot added the stale label Jun 3, 2026

github-actions Bot closed this Jun 25, 2026

panjwanirahul reopened this Aug 14, 2026

panjwanirahul force-pushed the fix-787-null-externalurl-cache-npe branch from da96d75 to d21403a Compare August 14, 2026 11:14

cla-bot Bot removed the cla-signed label Aug 14, 2026

panjwanirahul removed the stale label Aug 14, 2026

Avoid null cache writes for query routing mappings

53019af

Guard query-id cache writes in routing manager against null/empty values to prevent runtime failures when query history rows have been removed by retention.

panjwanirahul force-pushed the fix-787-null-externalurl-cache-npe branch from d21403a to 53019af Compare August 14, 2026 11:18

cla-bot Bot added the cla-signed label Aug 14, 2026

mosabua added the stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. label Aug 14, 2026

Uh oh!

Conversation

panjwanirahul commented Mar 15, 2026

What changed

Why

Additional context and related issues

Release notes

Uh oh!

cla-bot Bot commented Mar 15, 2026

Uh oh!

rahulpanjwani17 commented Mar 30, 2026

Uh oh!

ebyhr commented Mar 30, 2026

Uh oh!

cla-bot Bot commented Mar 30, 2026

Uh oh!

rahulpanjwani17 commented Mar 30, 2026

Uh oh!

panjwanirahul commented Apr 2, 2026

Uh oh!

panjwanirahul commented Apr 3, 2026

Uh oh!

ebyhr Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

panjwanirahul Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ebyhr Apr 3, 2026 • 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!

panjwanirahul Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

cla-bot Bot commented Apr 13, 2026

Uh oh!

panjwanirahul commented Apr 15, 2026

Uh oh!

panjwanirahul commented Apr 16, 2026

Uh oh!

panjwanirahul commented Apr 19, 2026

Uh oh!

vishalya left a comment

Choose a reason for hiding this comment

Uh oh!

panjwanirahul commented Apr 22, 2026

Uh oh!

vishalya commented Apr 24, 2026

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

panjwanirahul commented May 5, 2026

Uh oh!

panjwanirahul commented May 11, 2026

Uh oh!

Chaho12 commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 25, 2026

Uh oh!

cla-bot Bot commented Aug 14, 2026

Uh oh!

panjwanirahul commented Aug 14, 2026

Why this happens

Minimal unit-test reproduction (current main)

End-to-end reproduction

Local validation on latest main

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

ebyhr Apr 3, 2026 •

edited

Loading

Chaho12 commented May 13, 2026 •

edited

Loading