Make taskUpdateRequest and taskInfo classes Thrift ready with json fields #25020

shangm2 · 2025-04-30T20:49:20Z

Description

We are enabling thrift for task update request, and task info for critical api communication between coordinator and worker. We have two config toggles for task update request sent to worker and the task info returned to coordinator,
However, there are some classes that are java interface/polymorphic fields. We keep them as json encoding for now and will migrate them in the next step.
We are also doing proper change for native worker: [native] Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON fields in CPP #25079

Motivation and Context

We observed that coordinator can spend too much cpu/heap memory on json serde for taskUpdateRequest.

Impact

Test Plan

Passed verifier tests

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve communication between coordinator and worker with thrift serde.

presto-main-base/src/main/java/com/facebook/presto/execution/ExecutionFailureInfo.java

shangm2 · 2025-05-05T16:28:32Z

presto-spi/src/main/java/com/facebook/presto/spi/function/FunctionKind.java

-    SCALAR(1),
-    AGGREGATE(2),
-    WINDOW(3);
+    SCALAR(0),


Seems like cpp worker expects enum to start from 0. Vivian should have more context and will ask her to comment.

shangm2 · 2025-05-05T16:28:56Z

presto-spi/src/main/java/com/facebook/presto/spi/function/SqlFunctionId.java

    private final List<TypeSignature> argumentTypes;

    @ThriftConstructor
+    public SqlFunctionId(String signature)


Seems like cpp worker is expecting a string.

facebook-github-bot · 2025-05-05T18:08:28Z

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-05-05T18:25:07Z

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-05-05T18:42:17Z

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-05-05T21:08:31Z

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

shangm2 · 2025-05-08T05:41:25Z

presto-main-base/src/main/java/com/facebook/presto/server/thrift/CustomCodecUtils.java

+                        referencedType)),
+                Optional.empty());
+        return new ThriftStructMetadata(
+                originalType.getSimpleName() + "Wrapper",


The wrapper struct will look like this

shangm2 · 2025-05-08T18:23:21Z

presto-main-base/src/main/java/com/facebook/presto/server/thrift/CustomCodecUtils.java

+
+        ThriftFieldMetadata fieldMetaData = new ThriftFieldMetadata(
+                fieldId,
+                false, false, NONE, ImmutableMap.of(),


fbthrift gives the following waring if "REQUIRED" is used here for requireness in idl. Change it to NONE.

facebook-github-bot · 2025-05-09T19:25:58Z

@vhsu14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

tdcmeehan · 2025-05-12T15:45:12Z

However, there are some classes that are java interface/polymorphic fields. We keep them as json encoding for now and will migrate them in the next step.

How do we plan to handle these polymorphic types, given that many of them are connector-specific and not known upfront? Is there a design you can share?

shangm2 · 2025-05-14T17:25:13Z

However, there are some classes that are java interface/polymorphic fields. We keep them as json encoding for now and will migrate them in the next step.

How do we plan to handle these polymorphic types, given that many of them are connector-specific and not known upfront? Is there a design you can share?

Hey Tim. For connector-specific, we will use a similar design as HandleResolver.java. We will start with prism connector.
The thrift serialization will certainly be optional, and we also want to make it flexible for other options, say, if we want to move to other serde. As you may already know, for interfaces of which we already know all its implementation upfront, we will be using thrift union.

I am working with @amitkdutta to make a document that can be shared given that the company is monitoring all the typing/uploading/copy-paste thing now (maybe even before but it is getting worse).

Let me know if this helps and thanks for all the comments!

tdcmeehan · 2025-05-14T17:48:43Z

Thanks @shangm2. I'm not sure if this helps you, but I don't think you should copy over an internal document, this should be written as an RFC so the open source community can work with you on the design.

presto-main-base/pom.xml

presto-main-base/src/main/java/com/facebook/presto/server/thrift/ThriftCodecWrapper.java

shangm2 · 2025-05-16T18:12:54Z

However, there are some classes that are java interface/polymorphic fields. We keep them as json encoding for now and will migrate them in the next step.

How do we plan to handle these polymorphic types, given that many of them are connector-specific and not known upfront? Is there a design you can share?

Hey @tdcmeehan . I have a rfc ready for review. prestodb/rfcs#38

presto-main-base/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java

…th JSON fields in CPP (#25079) ## Description This PR depends on #25020. Changes include: - Use IDL generated by coordinator and make relevant changes to enable thrift for TaskStatus, TaskUpdateRequest, and TaskInfo on native worker. - Remove old pipeline of using python, json, and chevron templates for producing C++ code. ## Motivation and Context We observed that coordinator can spend too much cpu/heap memory on json serde for taskUpdateRequest. ## Impact  ## Test Plan Verifier ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] 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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Improve communication between coordinator and worker with thrift serde. ```

…elds (prestodb#25020) ## Description 1. We are enabling thrift for task update request, and task info for critical api communication between coordinator and worker. We have two config toggles for task update request sent to worker and the task info returned to coordinator, 2. However, there are some classes that are java interface/polymorphic fields. We keep them as json encoding for now and will migrate them in the next step. 3. We are also doing proper change for native worker: prestodb#25079 ## Motivation and Context 1. We observed that coordinator can spend too much cpu/heap memory on json serde for taskUpdateRequest. ## Impact  ## Test Plan 1. Passed verifier tests ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] 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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Improve communication between coordinator and worker with thrift serde. ```

…th JSON fields in CPP (prestodb#25079) ## Description This PR depends on prestodb#25020. Changes include: - Use IDL generated by coordinator and make relevant changes to enable thrift for TaskStatus, TaskUpdateRequest, and TaskInfo on native worker. - Remove old pipeline of using python, json, and chevron templates for producing C++ code. ## Motivation and Context We observed that coordinator can spend too much cpu/heap memory on json serde for taskUpdateRequest. ## Impact  ## Test Plan Verifier ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] 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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == General Changes * Improve communication between coordinator and worker with thrift serde. ```

shangm2 requested review from elharo and a team as code owners April 30, 2025 20:49

shangm2 requested a review from jaystarshot April 30, 2025 20:49

prestodb-ci added the from:Meta PR from Meta label Apr 30, 2025

shangm2 commented Apr 30, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/execution/ExecutionFailureInfo.java Show resolved Hide resolved

shangm2 changed the title ~~Maks TaskStatus, TaskUpdateRequest, TaskInfo Thrift ready with json fields~~ Maks TaskUpdateRequest, TaskInfo Thrift ready with json fields May 1, 2025

shangm2 changed the title ~~Maks TaskUpdateRequest, TaskInfo Thrift ready with json fields~~ Make taskUpdateRequest and taskInfo classes Thrift ready with json fields May 1, 2025

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch 15 times, most recently from 6c457e3 to 1f1b450 Compare May 5, 2025 16:23

shangm2 commented May 5, 2025

View reviewed changes

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch from 1f1b450 to d701020 Compare May 5, 2025 18:24

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch from d701020 to 1632a75 Compare May 5, 2025 21:06

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch 3 times, most recently from a811b10 to f9cb110 Compare May 8, 2025 01:31

shangm2 commented May 8, 2025

View reviewed changes

arhimondr previously approved these changes May 8, 2025

View reviewed changes

shangm2 dismissed arhimondr’s stale review via 2b918bf May 8, 2025 18:21

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch from f9cb110 to 2b918bf Compare May 8, 2025 18:21

shangm2 commented May 8, 2025

View reviewed changes

vhsu14 mentioned this pull request May 8, 2025

[native] Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON fields in CPP #25079

Merged

6 tasks

NikhilCollooru reviewed May 16, 2025

View reviewed changes

presto-main-base/pom.xml Outdated Show resolved Hide resolved

NikhilCollooru reviewed May 16, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/server/thrift/ThriftCodecWrapper.java Show resolved Hide resolved

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch 2 times, most recently from 2916c02 to 5e93c8b Compare May 16, 2025 19:02

NikhilCollooru reviewed May 16, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java Outdated Show resolved Hide resolved

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch from 5e93c8b to d396cab Compare May 16, 2025 19:54

Make TaskUpdateRequest, TaskInfo Thrift ready with json fields

8a9265a

shangm2 force-pushed the driftIDL_Generator_With_Mixed_ThriftAndJson branch from d396cab to 8a9265a Compare May 16, 2025 20:08

NikhilCollooru approved these changes May 16, 2025

View reviewed changes

shangm2 merged commit 9f77bed into prestodb:master May 19, 2025
98 checks passed

ZacBlanco mentioned this pull request May 29, 2025

Add release notes for 0.293 #25222

Merged

21 tasks

This was referenced May 30, 2025

Add release notes for 0.293 unix280/presto#31

Closed

Add release notes for 0.293 unix280/presto#32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make taskUpdateRequest and taskInfo classes Thrift ready with json fields #25020

Make taskUpdateRequest and taskInfo classes Thrift ready with json fields #25020

shangm2 commented Apr 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

shangm2 May 5, 2025

Uh oh!

shangm2 May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

shangm2 May 8, 2025 •

edited

Loading

Uh oh!

shangm2 May 8, 2025

Uh oh!

facebook-github-bot commented May 9, 2025

Uh oh!

tdcmeehan commented May 12, 2025

Uh oh!

shangm2 commented May 14, 2025

Uh oh!

tdcmeehan commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

shangm2 commented May 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Make taskUpdateRequest and taskInfo classes Thrift ready with json fields #25020

Make taskUpdateRequest and taskInfo classes Thrift ready with json fields #25020

Conversation

shangm2 commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

shangm2 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

shangm2 May 5, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

shangm2 May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangm2 May 8, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 9, 2025

Uh oh!

tdcmeehan commented May 12, 2025

Uh oh!

shangm2 commented May 14, 2025

Uh oh!

tdcmeehan commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

shangm2 commented May 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shangm2 commented Apr 30, 2025 •

edited

Loading

shangm2 May 8, 2025 •

edited

Loading