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

Drop legacy results api #21772

Merged
merged 2 commits into from
Jan 26, 2024
Merged

Drop legacy results api #21772

merged 2 commits into from
Jan 26, 2024

Conversation

arhimondr
Copy link
Member

Description

In favor of /async version. Point the original endpoint to /v1/task/async.

Motivation and Context

Async getResults API has been enabled by default

Test Plan

CI

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Deprecate exchange.async-page-transport-enabled configuration property

@arhimondr arhimondr requested a review from a team as a code owner January 24, 2024 17:23
@arhimondr
Copy link
Member Author

Still WIP

@arhimondr
Remove blocking implementation of /v1/task/<id>/results
16f963c 
Point the original endpoint to /v1/task/async
@arhimondr
Copy link
Member Author

Ready for review

@tdcmeehan tdcmeehan self-assigned this Jan 24, 2024
spershin
spershin reviewed Jan 24, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/server/TaskResource.java
Comment on lines -302 to -304
@Path("{taskId}/results/{bufferId}/{token}")
@Produces(PRESTO_PAGES)
public void getResults(
Copy link
Contributor

Choose a reason for hiding this comment

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

@arhimondr
Why is this going way?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an old implementation based on blocking IO. AsyncPageTransportServlet.java implements in in a non-blocking way. Semantic wise both are identical.

@tdcmeehan tdcmeehan linked an issue Jan 24, 2024 that may be closed by this pull request
Drop /v1/task endpoint in favor of /v1/task/async #21748
Closed
tdcmeehan
tdcmeehan reviewed Jan 24, 2024
View reviewed changes
presto-main/src/main/java/com/facebook/presto/server/AsyncPageTransportForwardFilter.java
* Only prefix or suffix matching is allowed (e.g.: /v1/task/*, *\/suffix).
* Hence a more nuanced matching and a forward is required.
*/
public class AsyncPageTransportForwardFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just implement the rename in one shot, as opposed to forwarding /v1/task to /v1/task/async? i.e. why not just use /v1/task and remove this filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then all the requests starting with /v1/task will be routed to the custom servlet, while requests other than /v1/task*/results/* have to be routed to TaskResource

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a better way of doing it.

tdcmeehan
tdcmeehan approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

LGTM

@arhimondr arhimondr merged commit 7290ab8 into prestodb:master Jan 26, 2024
56 checks passed
@arhimondr arhimondr deleted the drop-legacy-results-api branch January 26, 2024 18:46
@wanglinsong wanglinsong mentioned this pull request Feb 12, 2024
Merged
64 tasks
@mbasmanova mbasmanova mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spershin spershin spershin left review comments

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@amitkdutta amitkdutta Awaiting requested review from amitkdutta

Assignees

@tdcmeehan tdcmeehan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop /v1/task endpoint in favor of /v1/task/async
3 participants
@arhimondr @spershin @tdcmeehan