Skip to content

Comments

[native] Switch to use string based arbitrator configs#23496

Merged
tanjialiang merged 1 commit intoprestodb:masterfrom
tanjialiang:rm_arb_conf
Aug 23, 2024
Merged

[native] Switch to use string based arbitrator configs#23496
tanjialiang merged 1 commit intoprestodb:masterfrom
tanjialiang:rm_arb_conf

Conversation

@tanjialiang
Copy link
Contributor

@tanjialiang tanjialiang commented Aug 22, 2024

Description

As we move forward with adopting the new string based configurations for implementation specific arbitrator configs, we are removing the old configs and migrate the configs to the new shared arbitrator configs.

== RELEASE NOTES ==

Shared arbitrator related configs are renamed to the following:
shared-arbitrator.reserved-capacity
shared-arbitrator.memory-pool-initial-capacity
shared-arbitrator.global-arbitration-enabled
shared-arbitrator.memory-pool-reserved-capacity
shared-arbitrator.memory-pool-transfer-capacity
shared-arbitrator.memory-reclaim-max-wait-time
(new) shared-arbitrator.fast-exponential-growth-capacity-limit
(new) shared-arbitrator.slow-capacity-grow-pct
(new) shared-arbitrator.memory-pool-min-free-capacity
(new) shared-arbitrator.memory-pool-min-free-capacity-pct

@facebook-github-bot
Copy link
Collaborator

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

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 some nits of formatting, the content looks great.

zero, then there is no timeout.

``shared-arbitrator.fast-exponential-growth-capacity-limit``
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


When shared arbitrator grows memory pool's capacity, the growth bytes will
be adjusted in the following way:
- If 2 * current capacity is less than or equal to
Copy link
Contributor

Choose a reason for hiding this comment

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

/Users/steveburnett/Documents/GitHub/presto/presto-docs/src/main/sphinx/presto_cpp/properties.rst:211: ERROR: Unexpected indentation.

Adjust the indentation for this list until the gray vertical line in the screenshot does not appear in a local doc build. See Building the documentation in the presto-docs README for information on local doc builds.

Screenshot 2024-08-22 at 2 47 20 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just revised

xiaoxmeng
xiaoxmeng previously approved these changes Aug 22, 2024
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@tanjialiang LGTM. Thanks!

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.

Thank you for the quick revision! I took another look and found some more nits of formatting and phrasing, but I think this should be everything.

@tanjialiang
Copy link
Contributor Author

Thank you for the quick revision! I took another look and found some more nits of formatting and phrasing, but I think this should be everything.

Hi @steveburnett , just addressed your comments. Would you help to take another look? Thanks

@facebook-github-bot
Copy link
Collaborator

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

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 doc build, looks good. Thanks!

@tanjialiang tanjialiang merged commit d7ca7af into prestodb:master Aug 23, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:Meta PR from Meta

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants