Use external url for query routing history

[raj-manvar]

Description

Use external url field for query routing history page

Additional context and related issues

The backendUrl used by Trino gateway may not be accessible for users, instead the external url field is used to indicate the hostname that's accessible by Trino users.

For example, when hosted on Kubernetes, it's common to use Kubernetes Service for inter apps communication like between Gateway and Trino clusters, in such cases the hostname used by Gateway would be something like service-name.namespace.svc.cluster.local which is only accessible from within the Kubernetes cluster.

#81 is relevant issue with other discussion points.

Release notes

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

* Fix some things. ({issue}`issuenumber`)

[cla-bot]

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]

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

[xkrogen]

Big +1 on this, we actually just finished an internal implementation of this only a couple of weeks ago that we have been planning to open source. cc @felicity3786 can you help collaborate on this?

[felicity3786]

Overall LGTM!
Internally our implementation is a more "normalized" approache, we keep query_history without external_url, and join the gateway_backend to get it at the read time. It would leads to some potential mismatch if the gateway_backend table got updated, but no schema migration needed.
I feel in this PR, it's handled cleaner and simpler. Although there will be risk of "stale" data like if admins rename external_url, but given query_history will be purged periodically I don't think it would be a very big concern.
cc @xkrogen

[xkrogen]

Although there will be risk of "stale" data like if admins rename external_url, but given query_history will be purged periodically I don't think it would be a very big concern.

Agree that this could be a concern, but it applies even with the backend_url (i.e. even without this change, if we change the backend_url for a backend, the old history won't update). To actually tackle this, we would have to begin registering backends under some ID, and then store the backend's ID in the history instead of the URLs, and join with the backend table to reconstruct. So I think adding external_url is still fine, consistent with our current approach of the history table storing point-in-time info as of when the query was executed. If/when practical issues are observed that would encourage us to normalize this data, moving out both backend_url/external_url should be no more work than just moving backend_url.

[Chaho12]

Can you update the docs on explaning difference between RoutedTo and ExternalURL? Adding purpose of why 2 url exists would be great.
As you have noticed, external URL was not used at all before.

[raj-manvar]

Added documentation on two URLs, purposes and example for the same. cc: @Chaho12

[vishalya]

Thanks for filling the gap @raj-manvar. Could you also please add some unit tests in TestHaGatewayManager

• default value and setting explicit values
• towards cache hits and misses

[vishalya]

Approved.
Does it good to the rest of reviewers before I merge it?

[ebyhr]

Does it good to the rest of reviewers before I merge it?

Yes, please don't merge this PR yet.

[ebyhr]

Please squash commits and separate commits into "Use external url for query routing history" and other style changes.

[ebyhr]

Please rebase on main to resolve conflicts.

[raj-manvar]

Thanks @ebyhr for the help & feedback! I have rebased with main branch and also squashed all commits into a single commit

[ebyhr]

use external url for query routing history

Please capitalize the commit title
https://trino.io/development/process.html#pull-request-and-commit-guidelines-

[raj-manvar]

use external url for query routing history

Please capitalize the commit title https://trino.io/development/process.html#pull-request-and-commit-guidelines-

Thanks! I'll be careful for future commits. Updated current MR commit messages.

[Chaho12]

do we now need mapping[backend.proxyTo] = backend.name;? i think we can now remove that part in this code

        for (let index = 0; index < data.length; index++) {
          const backend = data[index] as BackendData;
          mapping[backend.name] = backend.proxyTo;
          mapping[backend.proxyTo] = backend.name;
          mapping[backend.externalUrl] = backend.name;
        }

[raj-manvar]

I think it's still used in getting the backend Name from backendUrl at https://github.com/trinodb/trino-gateway/blob/main/webapp/src/components/history.tsx#L125

It's also getting used at https://github.com/trinodb/trino-gateway/blob/main/webapp/src/components/history.tsx#L99, but we can replace this usage with just b.name ?
We could remove the remaining two mapping ( backend.name -> proxyTo and externalUrl -> name ), should we do it as part of this pull request though, as it's not strictly related to this change ? or create a separate PR ?

[Chaho12]

A separate PR sounds better :) Thx for checking logic.

[raj-manvar]

Thanks, I created #711 so that we don't miss this.

[Chaho12]

I think you misunderstood ebyhr comment on capitalizing commit message. We only capitalize the first letter of the subject line.
e.g) Use external URL for query routing history, Remove inferred types and this references

[raj-manvar]

I think you misunderstood ebyhr comment on capitalizing commit message. We only capitalize the first letter of the subject line. e.g) Use external URL for query routing history, Remove inferred types and this references

Ah thanks okay. I updated the commit messages accordingly.

[raj-manvar]

@ebyhr Does this good look to merge now ?

[mosabua]

I checked docs now .. all good there. @andythsu @ebyhr @vishalya @Chaho12 .. is this good to go? Please update your review notes.

[Chaho12]

LGTM 👍

[andythsu]

🚀 !

[raj-manvar]

@ebyhr @vishalya can you please help merge this.

[Chaho12]

Will rebase and merge in few hours if you don't have any further comments @ebyhr :)

[Chaho12]

Just for safety, I checked that rebase has no conflict :) so will rebase and merge