Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add option for custom v1/statement-like paths #326

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Add option for custom v1/statement-like paths #326

merged 1 commit into from
Sep 5, 2024

Conversation

willmostly
Copy link
Contributor

Description

Add an option to configure additional custom paths that should be treated like /v1/statement. This is required to support some features of Starburst Trino.

Additional context and related issues

The trino gateway checks if a request path starts with /v1/statement to decide if it should use query processing logic.

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:
Add option for custom v1/statement-like paths

@cla-bot cla-bot bot added the cla-signed label Apr 25, 2024
@willmostly
Copy link
Contributor Author

The runtime changes introduced here are limited. However, QueryIdCachingProxyHandler units tests were expanded to cover instance methods, which required some significant refactoring. The functionality of RoutingManager and ProxyHandlerStats should not be changed, but interfaces were extracted so that testing versions of these classes could be introduced.

@willmostly willmostly force-pushed the will/additional-statement-paths branch 3 times, most recently from 2d93a73 to 377be60 Compare April 26, 2024 03:15
@ebyhr
Copy link
Member

ebyhr commented Apr 29, 2024

This is required to support some features of Starburst Trino.

Is it possible to test the feature with https://hub.docker.com/r/starburstdata/starburst-enterprise?

@willmostly willmostly force-pushed the will/additional-statement-paths branch from 377be60 to cf72670 Compare May 2, 2024 16:37
mosabua
mosabua requested changes May 2, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Just some initial feedback after taking a glance.

gateway-ha/src/main/java/io/trino/gateway/ha/config/HaGatewayConfiguration.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/handler/DropWizardProxyHandlerStats.java Outdated Show resolved Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/handler/DropWizardProxyHandlerStats.java Outdated Show resolved Hide resolved
@willmostly
Copy link
Contributor Author

This is required to support some features of Starburst Trino.

Is it possible to test the feature with https://hub.docker.com/r/starburstdata/starburst-enterprise?

It is not, the Insights IDE feature that exposes the non-standard statement endpoint requires a license

@willmostly willmostly force-pushed the will/additional-statement-paths branch from cf72670 to ac1c19e Compare May 23, 2024 19:44
@willmostly willmostly requested a review from mosabua May 23, 2024 19:53
@mosabua
Copy link
Member

mosabua commented May 25, 2024

This is required to support some features of Starburst Trino.

Is it possible to test the feature with https://hub.docker.com/r/starburstdata/starburst-enterprise?

It is not, the Insights IDE feature that exposes the non-standard statement endpoint requires a license

Also we dont want to start testing with various distributions - that is something the distributors should do in their own CI setup.

@mosabua
Copy link
Member

mosabua commented May 29, 2024

Lets discuss in the dev sync tomorrow.. I think we should merge this after adding docs .. or get the airlift removal done first and refactor this after.

@mosabua
Copy link
Member

mosabua commented Jun 30, 2024

Another one to rebase and then merge @willmostly ;-)

@willmostly willmostly force-pushed the will/additional-statement-paths branch 3 times, most recently from 5481be9 to 3387e96 Compare August 16, 2024 00:03
@oneonestar oneonestar self-requested a review August 21, 2024 13:15
mosabua
mosabua reviewed Aug 21, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think the PR looks mostly good, however I think we need to add tests and potentially make the code more robust. I think we should allow something like the following as well

  • v1/statement
  • somerandomcontext/v1/statement

Or alternatively require the root / symbol and verify for it.

And with multiple also allows mixing like

  • /v1/statement, v1/statement, blabla/v2/statement, some/custom/path

Does that make sense?

Also.. we are not verifying at all if v1/statement is in the list and I think that is fine .. however do we want to log a warning about that? Or do we want to require is thinking that if that URL is not used its not a valid trino cluster or so .. I think thats wrong but feel free to convince me otherwise.

@willmostly willmostly force-pushed the will/additional-statement-paths branch 2 times, most recently from 6464921 to ac76176 Compare August 23, 2024 21:23
@willmostly
Copy link
Contributor Author

Also.. we are not verifying at all if v1/statement is in the list and I think that is fine .. however do we want to log a warning about that? Or do we want to require is thinking that if that URL is not used its not a valid trino cluster or so .. I think thats wrong but feel free to convince me otherwise.

/v1/statement is always added to the list. I'll document that behavior. Do you think changing the config name to additionalStatementPaths would be helpful?

I think we need to add tests and potentially make the code more robust

I have

assertThat(extractQueryIdIfPresent("/custom/api/statement/executing/20200416_160256_03078_6b4yt/ya7e884929c67cdf86207a80e7a77ab2166fa2e7b/1368", null, statementPaths))

in TestQueryIdCachingProxyHandler and tests for the config validation. Would you like to see an end-to-end test as well? I can mock up a backend with a custom endpoint.

@willmostly willmostly force-pushed the will/additional-statement-paths branch from ac76176 to 4ff272d Compare August 23, 2024 21:31
@mosabua
Copy link
Member

mosabua commented Aug 23, 2024 •
edited
Loading

No need for a mock test imho.

Renaming to addtionalStatementPaths might be good .. under the assumption that we agree that the v1/statement path has to always be there .. which I am not 100% sure of

@mosabua
Copy link
Member

mosabua commented Aug 26, 2024

Let me know when you refactoring the variable name @willmostly .. assuming thats what we are going with.

@willmostly willmostly force-pushed the will/additional-statement-paths branch from 4ff272d to 11bcc4f Compare September 3, 2024 20:36
@willmostly
Copy link
Contributor Author

@mosabua updated and added short doc section

mosabua
mosabua approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me now. Please check on the build failure though..

@willmostly
Copy link
Contributor Author

Please check on the build failure though..

I should have saved the error message, but the build failed because it downloaded a dependency and the archive file was corrupted. It succeeded on re-run

@willmostly willmostly force-pushed the will/additional-statement-paths branch from 11bcc4f to 5c14b7f Compare September 4, 2024 19:26
@mosabua
Copy link
Member

mosabua commented Sep 4, 2024

I think we should merge this .. any objections @willmostly @vishalya @Chaho12 @oneonestar ?

Chaho12
Chaho12 approved these changes Sep 5, 2024
View reviewed changes
Copy link
Member

@Chaho12 Chaho12 left a comment

Choose a reason for hiding this comment

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

LGTM

docs/installation.md
@@ -138,6 +138,23 @@ extraWhitelistPaths:
- '/ext/faster'
```

### Configure additional v1/statement-like paths

The Trino client protocol specifies that queries are initiated by a POST to `v1/statement`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The Trino client protocol specifies that queries are initiated by a POST to `v1/statement`.
The Trino client protocol specifies that queries are initiated by a POST to `/v1/statement`.

Might be good to say that path should start with / as it will give an error anyway

@willmostly
Copy link
Contributor Author

@mosabua no objections, lets do it!

@mosabua mosabua merged commit 6c57064 into trinodb:main Sep 5, 2024
3 checks passed
@github-actions github-actions bot added this to the 11 milestone Sep 5, 2024
@mosabua mosabua mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosabua mosabua mosabua approved these changes

@oneonestar oneonestar oneonestar approved these changes

@Chaho12 Chaho12 Chaho12 approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
11
Development

Successfully merging this pull request may close these issues.
5 participants
@willmostly @ebyhr @mosabua @oneonestar @Chaho12