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.

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. - Use direct static import for isNullOrEmpty. - Change log level from warn to debug for empty queryId checks. - Standardize null/empty handling across all cache setters. - Add regression tests for null/empty external URL and query ID. Co-authored-by: Cursor <cursoragent@cursor.com>

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

@ebyhr done, please check now

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

@ebyhr @mosabua please check this

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

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

ebyhr Apr 3, 2026 •

edited

Loading