Skip to content

Conversation

@huw0
Copy link
Member

@huw0 huw0 commented May 1, 2025

Description

Session.createViewSession creates a new Session that is used when analysing views.

Presently this does not pass through the trace token. This is breaking the use of views with our custom connectors which mandate that this token is populated.

Therefore this is a very simple PR to ensure that the trace token is populated in createViewSession if it is present on the outer session.

Release notes

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

## General
* Preserve client trace token in CREATE VIEW ({issue}`25716`)

@cla-bot cla-bot bot added the cla-signed label May 1, 2025
@huw0 huw0 marked this pull request as ready for review May 1, 2025 20:00
@Praveen2112 Praveen2112 requested review from dain, kokosing and ksobolew May 2, 2025 13:56
@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from ab8ebfe to 323b87a Compare May 2, 2025 17:42
Copy link
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks simple enough. Is it possible to add a test?

@dain
Copy link
Member

dain commented May 5, 2025

This looks good to me, but if a test is possible, please add that. If not (or really annoying), please let us know, and one of us will merge it as is.

@Praveen2112
Copy link
Member

@huw0 For testing I think we could use MockConnectorMetadata which could make use of the trace token at the time of its execution.

@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from f82bae1 to 5e91edf Compare May 12, 2025 20:44
@huw0
Copy link
Member Author

huw0 commented May 12, 2025

I've added some unit testing around createViewSession and rebased the branch.
There seem to be some test failures caused by github rate limiting and some random plugin failures that change every time. Expect these should pass once any wider issues resolve.

@Praveen2112
Copy link
Member

Instead of this test - WDYT about #25716 (comment) ?

@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from 5e91edf to 4444b50 Compare May 18, 2025 15:26
@huw0
Copy link
Member Author

huw0 commented May 18, 2025

@Praveen2112 - I've added an additional commit with an alternative test using the approach you've outlined.


I've realised that this new test isn't actually hitting the createViewSession code. It looks like this could be done using MockConnectorPlugin as well? But seems that this creates a very bloated test?
I'd be in favour of dropping this 3rd commit and sticking with the first two. The unit test alone increases the existing coverage.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale label Jun 11, 2025
@huw0
Copy link
Member Author

huw0 commented Jun 16, 2025

I'd be in favour of dropping this 3rd commit and sticking with the first two. The unit test alone increases the existing coverage.

@dain @Praveen2112 - Please see comment above. I'd like to get this merged soon if there are no objections?

@github-actions github-actions bot removed the stale label Jun 17, 2025
@github-actions
Copy link

github-actions bot commented Jul 9, 2025

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

@github-actions github-actions bot added the stale label Jul 9, 2025
@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch 2 times, most recently from 509e048 to 26a60ba Compare July 11, 2025 11:53
@huw0 huw0 removed the stale label Jul 11, 2025
@huw0
Copy link
Member Author

huw0 commented Jul 11, 2025

Dropped 3rd commit and rebased.

@github-actions
Copy link

github-actions bot commented Aug 1, 2025

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

@github-actions github-actions bot added the stale label Aug 1, 2025
@github-actions
Copy link

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

@github-actions github-actions bot closed this Aug 25, 2025
@huw0 huw0 reopened this Aug 25, 2025
@github-actions github-actions bot removed the stale label Aug 26, 2025
@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from 26a60ba to 156b813 Compare September 8, 2025 14:11
@huw0
Copy link
Member Author

huw0 commented Sep 8, 2025

PR updated against latest master - but otherwise unchanged.

@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from 156b813 to 1239b95 Compare September 8, 2025 15:44
@raunaqmorarka raunaqmorarka merged commit dbe02cb into trinodb:master Sep 8, 2025
68 of 69 checks passed
@huw0
Copy link
Member Author

huw0 commented Sep 8, 2025

Thanks @raunaqmorarka for review and merge.

@github-actions github-actions bot added this to the 477 milestone Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

6 participants