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

vhsu14 · 2025-05-08T21:40:31Z

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, 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.

facebook-github-bot · 2025-05-08T21:40:57Z

This pull request was exported from Phabricator. Differential Revision: D72886878

facebook-github-bot · 2025-05-09T21:59:36Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-10T00:22:53Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-12T23:03:35Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079)

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-13T07:10:11Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079)

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-14T02:22:31Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-15T04:12:38Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

facebook-github-bot · 2025-05-15T04:16:18Z

This pull request was exported from Phabricator. Differential Revision: D72886878

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

presto-native-execution/presto_cpp/main/tests/TaskInfoTest.cpp

presto-native-execution/presto_cpp/main/tests/data/Fragment.txt

arhimondr · 2025-05-15T19:14:05Z

presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.h

+
+namespace facebook::presto::thrift {
+
+void toThrift(const std::string& proto, std::string& thrift);


Are ProtocolToThrift files auto generated?

no, the changes in ProtocolToThrift files are now hand-written. there used to be a complicated workflow to use python and chevron templates to auto-gen it, but it does not support some functionalities and we removed it as part of this PR.

Oh, I think I misread. This class is responsible for creating Thrift objects from presto::protocol objects and the other way around. The thrift objects are auto generated, right?

arhimondr

A couple of questions, otherwise looks good to me

presto-native-execution/presto_cpp/main/TaskResource.cpp

arhimondr · 2025-05-15T20:42:42Z

presto-native-execution/presto_cpp/main/TaskResource.cpp

-  auto acceptHeader = headers.getSingleOrEmpty(proxygen::HTTP_HEADER_ACCEPT);
-  auto useThrift =
+  const auto& acceptHeader = headers.getSingleOrEmpty(proxygen::HTTP_HEADER_ACCEPT);
+  const auto sendThrift =


Do we need to do the same for TaskInfo?

Discussed offline. Needs Java changes. Will be added as a follow up

arhimondr · 2025-05-15T20:44:14Z

presto-native-execution/presto_cpp/main/thrift/ProtocolToThrift.h

+
+namespace facebook::presto::thrift {
+
+void toThrift(const std::string& proto, std::string& thrift);


Oh, I think I misread. This class is responsible for creating Thrift objects from presto::protocol objects and the other way around. The thrift objects are auto generated, right?

arhimondr · 2025-05-15T20:45:09Z

presto-native-execution/presto_cpp/main/thrift/presto_thrift.thrift

 enum ErrorType {
  USER_ERROR = 0,
  INTERNAL_ERROR = 1,
  INSUFFICIENT_RESOURCES = 2,
  EXTERNAL = 3,
 }
-
+enum ErrorCause {


This file is auto generated based on idl, right?

Discussed offline.

The IDL is generated based on Java classes. IDL will be generated automatically once we land changes to Drift. CMake generates thrift:: protocol classes during build process, they are not commited.

As a follow up we will also be generating the ProtocolToThrift.h

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Reviewed By: yuandagits Differential Revision: D72886878

facebook-github-bot · 2025-05-15T22:14:33Z

This pull request was exported from Phabricator. Differential Revision: D72886878

tdcmeehan · 2025-05-19T13:17:33Z

@vhsu14 can you help to describe clearly how the code generation works? Based on the PR description, the coordinator somehow does this--can you explain?

shangm2 · 2025-05-19T16:16:41Z

@vhsu14 can you help to describe clearly how the code generation works? Based on the PR description, the coordinator somehow does this--can you explain?

@tdcmeehan we are using the idl generator from drift to generate the idl file from drift annotation. And we use the generate d idl file to generate cpp classes. This step is still manual and we will make it automatic in next step. There are also some issues with current pipeline, the one which use python scripts to do code generation, on how to support optional fields in thrift. And we will fix this in next step as well. Hope this answer your question.

…elds (#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: #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. ```

## Description  In #25079, ProtocolToThrift files were manually written. This PR generates these files using pythons scripts and chevron templates. ## Motivation and Context   ## Impact  ## Test Plan  Unit test + Verifier (test_id 225961) ## 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 * Update ProtocolToThrift files to be generated for cpp 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. ```

vhsu14 requested a review from a team as a code owner May 8, 2025 21:40

facebook-github-bot added the fb-exported label May 8, 2025

shangm2 changed the title ~~Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON fields in CPP~~ [native] Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON fields in CPP May 8, 2025

vhsu14 force-pushed the export-D72886878 branch 4 times, most recently from 283fdee to 56f1f9e Compare May 9, 2025 02:45

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 9, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

6bb7dd4

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from fcc64a3 to 6bb7dd4 Compare May 9, 2025 21:59

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 10, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

93997c0

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from 6bb7dd4 to 93997c0 Compare May 10, 2025 00:23

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 12, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

89a3461

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from 93997c0 to 89a3461 Compare May 12, 2025 23:03

shangm2 pushed a commit to shangm2/presto that referenced this pull request May 13, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

227f8b5

…ields in CPP (prestodb#25079)

shangm2 mentioned this pull request May 13, 2025

[test, no review][native] CMake test for CPP and clean up existing pipeline #25097

Closed

6 tasks

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 13, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

69f7779

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from 89a3461 to 69f7779 Compare May 13, 2025 05:39

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 13, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

fa072a5

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from 69f7779 to fa072a5 Compare May 13, 2025 07:10

shangm2 pushed a commit to shangm2/presto that referenced this pull request May 13, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

bdd641a

…ields in CPP (prestodb#25079)

shangm2 pushed a commit to shangm2/presto that referenced this pull request May 13, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

9bfd860

…ields in CPP (prestodb#25079)

vhsu14 force-pushed the export-D72886878 branch from fa072a5 to 942e648 Compare May 14, 2025 02:22

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 14, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

942e648

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 14, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

891f914

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 15, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

bd1aaaf

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from 2c09f18 to bd1aaaf Compare May 15, 2025 04:12

vhsu14 added a commit to vhsu14/presto that referenced this pull request May 15, 2025

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

014f577

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Differential Revision: D72886878

vhsu14 force-pushed the export-D72886878 branch from bd1aaaf to 014f577 Compare May 15, 2025 04:16

yuandagits reviewed May 15, 2025

View reviewed changes

presto-native-execution/presto_cpp/main/tests/TaskInfoTest.cpp Outdated Show resolved Hide resolved

yuandagits previously approved these changes May 15, 2025

View reviewed changes

arhimondr reviewed May 15, 2025

View reviewed changes

Migrate TaskStatus, TaskUpdateRequest, TaskInfo to Thrift with JSON f…

12fa224

…ields in CPP (prestodb#25079) Summary: Pull Request resolved: prestodb#25079 Reviewed By: yuandagits Differential Revision: D72886878

vhsu14 dismissed yuandagits’s stale review via 12fa224 May 15, 2025 22:14

vhsu14 force-pushed the export-D72886878 branch from 014f577 to 12fa224 Compare May 15, 2025 22:14

arhimondr approved these changes May 15, 2025

View reviewed changes

gggrace14 approved these changes May 16, 2025

View reviewed changes

shangm2 merged commit 5b4b71c into prestodb:master May 19, 2025
105 of 107 checks passed

vhsu14 mentioned this pull request May 20, 2025

Generate ProtocolToThrift files through make and commit #25162

Merged

6 tasks

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

steveburnett mentioned this pull request Jun 5, 2025

[docs] need doc for "Improve communication between coordinator and worker with thrift serde" #25255

Open

steveburnett mentioned this pull request Jun 6, 2025

[docs] Documentation for enabling Thrift serialization #25259

Open

shangm2 mentioned this pull request Jun 12, 2025

Add RFC to add thrift support for task status/updateRequest/info prestodb/rfcs#38

Open


		namespace facebook::presto::thrift {

		void toThrift(const std::string& proto, std::string& thrift);

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

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

Conversation

vhsu14 commented May 8, 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!

facebook-github-bot commented May 8, 2025

Uh oh!

facebook-github-bot commented May 9, 2025

Uh oh!

facebook-github-bot commented May 10, 2025

Uh oh!

facebook-github-bot commented May 12, 2025

Uh oh!

facebook-github-bot commented May 13, 2025

Uh oh!

facebook-github-bot commented May 14, 2025

Uh oh!

facebook-github-bot commented May 15, 2025

Uh oh!

facebook-github-bot commented May 15, 2025

Uh oh!

Uh oh!

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

vhsu14 May 15, 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!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

arhimondr May 15, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 15, 2025

Uh oh!

tdcmeehan commented May 19, 2025

Uh oh!

shangm2 commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

vhsu14 commented May 8, 2025 •

edited

Loading

vhsu14 May 15, 2025 •

edited

Loading