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

Add properties, requirements, and best practices from FTE blog #12231

Merged
merged 1 commit into from
May 9, 2022

Conversation

jhlodin
Copy link
Contributor

@jhlodin jhlodin commented May 3, 2022

Description

Add new information gleaned from the Tardigrade blog post PR (trinodb/trino.io#287) to the documentation

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Documentation

How would you describe this change to a non-technical end user or system administrator?

Add new information to the fault-tolerant execution and query management properties documentation

Related issues, pull requests, and links

Incorporates information from trinodb/trino.io#287

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@@ -49,7 +49,8 @@ execution on a Trino cluster:
* - ``exchange.deduplication-buffer-size``
- Size of the coordinator's in-memory buffer used by fault-tolerant
execution to store output of query :ref:`stages <trino-concept-stage>`.
If this buffer is filled during query execution, the query fails unless
If this buffer is filled during query execution, the query fails with a
"Task descriptor storage capacity has been exceeded" error message unless
Copy link
Member

Choose a reason for hiding this comment

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

Should we use code highlighting for the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worry it'll cause issues with SEO, which is the whole reason to have the full error text here.

Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

Left some comments. Thanks @jhlodin for working on this!


* Set the ``query.low-memory-killer.policy``
:doc:`query management property </admin/properties-query-management>` to
``total-reservation-on-blocked-nodes``, or queries may
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the docs are correct, then yes. Still worth calling out I think, because it's a property that a reader might not associate with FTE and have configured differently.


* The ``query.hash-partition-count`` :doc:`query management property
</admin/properties-query-management>` must be set to a value of ``50`` or
lower in ``config.properties``, as the filesystem exchanage manager supports
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about introducing a dedicated property for fault tolerant execution with an appropriate default. The discussion is happening here: #12228 (comment)

@jhlodin jhlodin force-pushed the jl/fte-blog-improvements branch from 8b5bf2b to 0b1db37 Compare May 5, 2022 18:44
@jhlodin
Copy link
Contributor Author

jhlodin commented May 5, 2022

Added the new property from #12263 to this PR. @arhimondr

@mosabua
Copy link
Member

mosabua commented May 5, 2022

Could we this merged with the code changes @arhimondr ?

@arhimondr arhimondr merged commit ffb09f9 into trinodb:master May 9, 2022
@github-actions github-actions bot added this to the 381 milestone May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants