Skip to content

Add requestDataSizesMaxWaitSec to Prestissimo system configs#24774

Merged
HeidiHan0000 merged 1 commit intoprestodb:masterfrom
HeidiHan0000:export-D71572165
Mar 28, 2025
Merged

Add requestDataSizesMaxWaitSec to Prestissimo system configs#24774
HeidiHan0000 merged 1 commit intoprestodb:masterfrom
HeidiHan0000:export-D71572165

Conversation

@HeidiHan0000
Copy link
Contributor

Summary: as title

Differential Revision: D71572165

@HeidiHan0000 HeidiHan0000 requested review from a team, elharo and steveburnett as code owners March 22, 2025 01:03
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Mar 22, 2025
@facebook-github-bot
Copy link
Collaborator

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

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the doc! Just one nit.

Copy link
Collaborator

@kewang1024 kewang1024 left a comment

Choose a reason for hiding this comment

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

Thanks @HeidiHan0000 for making the changes


// Max wait time for exchange request in seconds.
static constexpr std::string_view kRequestDataSizesMaxWaitSec{
"request-data-sizes-max-wait-sec"};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change it to exchange.http-client.request-data-sizes-max-wait-sec


The periodic duration to apply cache TTL and evict AsyncDataCache and SSD cache entries.

``request-data-sizes-max-wait-sec``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a new categorization?

Exchange Properties
----------------

HeidiHan0000 added a commit to HeidiHan0000/presto that referenced this pull request Mar 24, 2025
@facebook-github-bot
Copy link
Collaborator

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

@HeidiHan0000 HeidiHan0000 requested a review from kewang1024 March 24, 2025 23:26
HeidiHan0000 added a commit to HeidiHan0000/presto that referenced this pull request Mar 25, 2025
@facebook-github-bot
Copy link
Collaborator

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

HeidiHan0000 added a commit to HeidiHan0000/presto that referenced this pull request Mar 25, 2025
@facebook-github-bot
Copy link
Collaborator

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

HeidiHan0000 added a commit to HeidiHan0000/presto that referenced this pull request Mar 25, 2025
…b#24774)

Summary:
Pull Request resolved: prestodb#24774

as title

Differential Revision: D71572165
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

A nit of formatting that I overlooked in yesterday's review. Sorry about that!

Comment on lines 57 to 58
{core::QueryConfig::kRequestDataSizesMaxWaitSec,
std::string(SystemConfig::kRequestDataSizesMaxWaitSec)}};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest leaving the session property change to next PR together with java side change, and we should move this to SessionProperties which is the centralized place for placing session property

{"aggregation_spill_all", "true"},
{"native_expression_max_array_size_in_reduce", "99999"},
{"native_expression_max_compiled_regexes", "54321"},
{"request_data_sizes_max_wait_sec", "20"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can leave the session property change to next PR

@facebook-github-bot
Copy link
Collaborator

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

Copy link
Contributor

@steveburnett steveburnett 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)

Pull updated branch, new local doc build, looks good. Thanks!

@HeidiHan0000 HeidiHan0000 merged commit 34e068c into prestodb:master Mar 28, 2025
100 of 102 checks passed
@prestodb-ci prestodb-ci mentioned this pull request Mar 28, 2025
30 tasks
pradeepvaka pushed a commit to pradeepvaka/presto that referenced this pull request Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants