Accept query id and slug in QueuedStatement #23407

prithvip · 2024-08-08T21:18:08Z

Description

This change adds support for QueuedStatementResource to accept a pre-minted query id and slug.

Motivation and Context

This is required for RFC5 prestodb/rfcs#23

Impact

There is no user-facing or performance impact.

Test Plan

Added test in TestServer

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
* Add support in QueuedStatement protocol to accept pre-minted query id and slug :pr:`23407`

rschlussel

Add a release note about the protocol change.

prithvip · 2024-08-12T21:00:35Z

Add a release note about the protocol change.

@rschlussel done

tdcmeehan

I think we can add some documentation here in statement.rst.

tdcmeehan · 2024-08-13T14:27:24Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

+            String statement,
+            @PathParam("queryId") QueryId queryId,
+            @QueryParam("slug") String slug,
+            @DefaultValue("false") @QueryParam("binaryResults") boolean binaryResults,


This was added for fuzzer testing, and no client presently uses this. I'd remove it.

I'm not sure that no client is using it, and I kept it here since the POST supports it. If we want to remove support for this, we should probably raise an issue first and remove it as part of a separate PR.

I asked around and it looks like we are actually using this flag for our client, so we should keep it.

Can you share more details on the client that's using it? Is it an internal client?

@amitkdutta do you have any details on this client?

tdcmeehan · 2024-08-13T14:28:54Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

+                tracerProviderManager.getTracerProvider(),
+                Optional.of(sessionPropertyManager));
+        Query query = new Query(statement, sessionContext, dispatchManager, queryResultsProvider, 0, queryId, slug);
+        queries.put(query.getQueryId(), query);


The best thing to do, to preserve idempotency while also ensuring that retries work, would be to check if the query already exists. If it does, check if the slug matches. If it does, then ensure that the token should be 0, otherwise, fail.

If the query doesn't already exist, then proceed as here.

I just remembered that POST is not an idempotent HTTP verb. Idempotency is a nice attribute, and I'd recommend using PUT instead, which is. (The other endpoint doesn't care about idempotency, because you can call POST multiple times for the same query, but the only one that matters is the first one whose nextUri is followed. Not so here--creating multiple queries with the same identical query ID is impossible.)

I still recommend the improvement above, because I think the API should guard against adversarial usage, even if it's trivial or unlikely. For example, POSTing an already executing query here would at best cause an error, at worst could be a security issue.

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

tdcmeehan · 2024-08-14T18:12:01Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

+     * @return {@link javax.ws.rs.core.Response} HTTP response code
+     */
+    @POST
+    @Path("/v1/statement/queued/{queryId}")


Suggested change

@Path("/v1/statement/queued/{queryId}")

@Path("/v1/statement/{queryId}")

Can we make this consistent with the POST? I don't think the queued signifies any additional information.

ZacBlanco

It would be great to have this added to the OpenAPI spec we're working on getting into the docs:

https://github.com/prestodb/presto/blob/master/presto-openapi/src/main/resources/queued_statement.yaml

prithvip · 2024-08-16T20:08:19Z

It would be great to have this added to the OpenAPI spec we're working on getting into the docs:

https://github.com/prestodb/presto/blob/master/presto-openapi/src/main/resources/queued_statement.yaml

@ZacBlanco Is there a README for how to add and test this?

ZacBlanco · 2024-08-16T21:26:47Z

We don't currently have a readme but it wouldn't hurt to have one. I'll try to submit a PR for that soon with instructions for adding new paths and components.

For the time being, I would look at the existing endpoints in the spec and try mimicking them for your new endpoint. You can refer the official spec for the full set of options: https://spec.openapis.org/oas/latest.html

Once you've added a new path to the YAML you can build the openapi module with ./mvnw install -pl presto-openapi and check the generated index.html in the target/ directory has your new endpoint that would be enough. We don't have tests but the build will error if your new additions to the YAML don't align with the OpenAPI spec.

prithvip · 2024-08-22T21:02:02Z

@tdcmeehan Addressed all comments:

Changed verb from POST to PUT
Added idempotency checks and tests for same
Added documentation to statement.rst

@ZacBlanco
Added docs to OpenAPI

steveburnett

LGTM! (docs)

Pull branch, local docs build, looks good. Thanks!

tdcmeehan

Looks good % one more comment

tdcmeehan · 2024-08-23T15:32:20Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

+        if (queries.containsKey(queryId)) {
+            Query query = queries.get(queryId);
+            if (!query.getSlug().equals(slug) || query.getLastToken() != 0) {
+                throw badRequest(CONFLICT, "Query already exists");
+            }
+        }
+
+        abortIfPrefixUrlInvalid(xPrestoPrefixUrl);


Suggested change

if (queries.containsKey(queryId)) {

Query query = queries.get(queryId);

if (!query.getSlug().equals(slug) || query.getLastToken() != 0) {

throw badRequest(CONFLICT, "Query already exists");

}

}

abortIfPrefixUrlInvalid(xPrestoPrefixUrl);

abortIfPrefixUrlInvalid(xPrestoPrefixUrl);

if (queries.containsKey(queryId)) {

Query query = queries.get(queryId);

if (!query.getSlug().equals(slug) || query.getLastToken() != 0) {

throw badRequest(CONFLICT, "Query already exists");

}

Query query = queries.get(queryId);

return withCompressionConfiguration(Response.ok(query.getInitialQueryResults(uriInfo, xForwardedProto, xPrestoPrefixUrl, binaryResults)), compressionEnabled).build();

}

I'd recommend returning the same query ID instead of proceeding and creating a new one, to guard against a race condition where we've retried the PUT, but meanwhile a GET has been made on the old copy of the query.

@tdcmeehan Done, changed to return the same query ID.

tdcmeehan · 2024-08-23T19:19:59Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

+        Query query = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);
+        queries.put(query.getQueryId(), query);


One more thing just to make it 100% foolproof:

Suggested change

Query query = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);

queries.put(query.getQueryId(), query);

Query query = queries.computeIfAbsent(queryId, unused -> new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug));

We could combine them into one check, and remove the check you have above.

Suggested change

Query query = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);

queries.put(query.getQueryId(), query);

Query attemptedQuery = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);

Query query = queries.putIfAbsent(query.getQueryId(), unused -> attemptedQuery);

if (attemptedQuery != query && !attemptedQuery.getSlug().equals(query.getSlug()) || query.getLastToken() != 0) {

throw badRequest(CONFLICT, "Query already exists");

}

prithvip · 2024-08-26T17:11:20Z

@ZacBlanco could I get a stamp? I've added the requested OpenAPI documentation.

ZacBlanco

Thanks for adding this! Sorry for the slow review. Was OOO last week.

Spec looks good, just need to shift the path component it is under.

presto-openapi/src/main/resources/queued_statement.yaml

prithvip · 2024-08-29T15:15:10Z

@tdcmeehan Could I get another stamp?

prithvip requested a review from a team as a code owner August 8, 2024 21:18

prithvip requested review from presto-oss and rschlussel August 8, 2024 21:18

rschlussel previously approved these changes Aug 12, 2024

View reviewed changes

tdcmeehan requested changes Aug 13, 2024

View reviewed changes

elharo reviewed Aug 14, 2024

View reviewed changes

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java Show resolved Hide resolved

tdcmeehan reviewed Aug 14, 2024

View reviewed changes

ZacBlanco requested changes Aug 16, 2024

View reviewed changes

prithvip dismissed rschlussel’s stale review via 2697a94 August 22, 2024 21:00

prithvip force-pushed the accept-query-id-and-slug branch from 5d2c3f0 to 2697a94 Compare August 22, 2024 21:00

prithvip requested a review from steveburnett as a code owner August 22, 2024 21:00

steveburnett previously approved these changes Aug 22, 2024

View reviewed changes

tdcmeehan requested changes Aug 23, 2024

View reviewed changes

prithvip dismissed steveburnett’s stale review via 3e495d2 August 23, 2024 19:10

prithvip force-pushed the accept-query-id-and-slug branch from 2697a94 to 3e495d2 Compare August 23, 2024 19:10

tdcmeehan requested changes Aug 23, 2024

View reviewed changes

prithvip force-pushed the accept-query-id-and-slug branch from 3e495d2 to f6201c7 Compare August 23, 2024 22:44

tdcmeehan previously approved these changes Aug 23, 2024

View reviewed changes

prithvip requested a review from ZacBlanco August 23, 2024 23:03

ZacBlanco requested changes Aug 26, 2024

View reviewed changes

presto-openapi/src/main/resources/queued_statement.yaml Show resolved Hide resolved

Accept query id and slug in QueuedStatement

02c294f

prithvip dismissed tdcmeehan’s stale review via 02c294f August 29, 2024 14:45

prithvip force-pushed the accept-query-id-and-slug branch from f6201c7 to 02c294f Compare August 29, 2024 14:45

ZacBlanco approved these changes Aug 29, 2024

View reviewed changes

tdcmeehan approved these changes Aug 29, 2024

View reviewed changes

prithvip merged commit 72c07cc into prestodb:master Aug 29, 2024
57 checks passed

jaystarshot mentioned this pull request Nov 1, 2024

Add release notes for 0.290 #23936

Open

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept query id and slug in QueuedStatement #23407

Accept query id and slug in QueuedStatement #23407

prithvip commented Aug 8, 2024 •

edited

Loading

rschlussel left a comment

prithvip commented Aug 12, 2024

tdcmeehan left a comment

tdcmeehan Aug 13, 2024

prithvip Aug 22, 2024

prithvip Aug 22, 2024

tdcmeehan Aug 23, 2024

prithvip Aug 23, 2024

tdcmeehan Aug 13, 2024

tdcmeehan Aug 14, 2024 •

edited

Loading

tdcmeehan Aug 14, 2024

ZacBlanco left a comment

prithvip commented Aug 16, 2024

ZacBlanco commented Aug 16, 2024 •

edited

Loading

prithvip commented Aug 22, 2024

steveburnett left a comment

tdcmeehan left a comment

tdcmeehan Aug 23, 2024

prithvip Aug 23, 2024

tdcmeehan Aug 23, 2024 •

edited

Loading

tdcmeehan Aug 23, 2024 •

edited

Loading

prithvip commented Aug 26, 2024

ZacBlanco left a comment

prithvip commented Aug 29, 2024

	@Path("/v1/statement/queued/{queryId}")
	@Path("/v1/statement/{queryId}")

		Query query = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);
		queries.put(query.getQueryId(), query);

	Query query = new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug);
	queries.put(query.getQueryId(), query);
	Query query = queries.computeIfAbsent(queryId, unused -> new Query(statement, sessionContext, dispatchManager, executingQueryResponseProvider, 0, queryId, slug));

Accept query id and slug in QueuedStatement #23407

Accept query id and slug in QueuedStatement #23407

Conversation

prithvip commented Aug 8, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

rschlussel left a comment

Choose a reason for hiding this comment

prithvip commented Aug 12, 2024

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

prithvip commented Aug 16, 2024

ZacBlanco commented Aug 16, 2024 • edited Loading

prithvip commented Aug 22, 2024

steveburnett left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

tdcmeehan Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

prithvip commented Aug 26, 2024

ZacBlanco left a comment

Choose a reason for hiding this comment

prithvip commented Aug 29, 2024

prithvip commented Aug 8, 2024 •

edited

Loading

tdcmeehan Aug 14, 2024 •

edited

Loading

ZacBlanco commented Aug 16, 2024 •

edited

Loading

tdcmeehan Aug 23, 2024 •

edited

Loading

tdcmeehan Aug 23, 2024 •

edited

Loading