Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Add Presto C++ config properties doc #22885

Merged

Conversation

steveburnett
Copy link
Contributor

@steveburnett steveburnett commented May 31, 2024

Description

Add documentation of Presto C++ configuration properties for coordinator and worker to existing Presto C++ documentation in Presto.

Motivation and Context

Fixes #22877.

Impact

Documentation.

Test Plan

Local docs build. Also CI.

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

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

== RELEASE NOTES ==

General Changes
* Add :doc:`/presto_cpp/properties` documentation :pr:`22885`

@steveburnett
Copy link
Contributor Author

Reviewers, please help me with the content - especially anything currently marked Default or Definition as placeholders. Thank you!

presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

Thank you @imjalpreet for the review! I have updated with a new (squashed) commit, and I believe I have addressed all your comments. Let me know if I missed anything.

@steveburnett
Copy link
Contributor Author

@agrawalreetika, @majetideepak, @aditi-pandit, @imjalpreet, @tdcmeehan: Thanks for your quick reviews! I have addressed the feedback from everyone and pushed (and squashed) an update.

I ask that you all take another look and re-review as you feel appropriate, please.

@steveburnett
Copy link
Contributor Author

@agrawalreetika, @majetideepak, @aditi-pandit, @imjalpreet, @tdcmeehan - I have addressed the feedback from everyone as best I understand the consensus, and I have pushed (and squashed) an update.

Please review the latest version and let me know what changes are needed.

Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@steveburnett 2 comments.

presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor Author

@majetideepak and @aditi-pandit, I've revised addressing your most recent discussions. Let me know if you'd like any more changes, please.

majetideepak
majetideepak previously approved these changes Jun 6, 2024
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

thanks, @steveburnett

@steveburnett
Copy link
Contributor Author

@agrawalreetika, I believe I addressed all of your requested changes. Would you review this again and let me know if you want anything else?

agrawalreetika
agrawalreetika previously approved these changes Jun 6, 2024
aditi-pandit
aditi-pandit previously approved these changes Jun 6, 2024
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @steveburnett for the rewrites.

imjalpreet
imjalpreet previously approved these changes Jun 9, 2024
@steveburnett
Copy link
Contributor Author

@majetideepak and @tdcmeehan, I have pushed a squashed commit update that removes the session properties from the doc. Take another look and let me know what you think, please.

Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Thanks @steveburnett !

@aditi-pandit aditi-pandit merged commit fb2a314 into prestodb:master Jun 10, 2024
57 checks passed
@steveburnett steveburnett deleted the steveburnett-cpp-properties branch June 10, 2024 19:06
@steveburnett
Copy link
Contributor Author

Thank you @agrawalreetika, @majetideepak, @aditi-pandit, @imjalpreet, and @tdcmeehan for your help!

@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clarify Presto C++ documentation to include required coordinator configuration properties
6 participants