Skip to content

Comments

Make max requests configurable in Azure native FS#22932

Merged
wendigo merged 1 commit intotrinodb:masterfrom
nineinchnick:azure-max-connections-config
Sep 18, 2024
Merged

Make max requests configurable in Azure native FS#22932
wendigo merged 1 commit intotrinodb:masterfrom
nineinchnick:azure-max-connections-config

Conversation

@nineinchnick
Copy link
Member

Description

Max requests were increased from 5 to 2 * number of CPUs in
bd289dc (#22561). With a very large number of
nodes in a cluster, this can cause throttling and result in failed
queries. Make it configurable allows to tune the value, if the default
one is too big.

This change also sets the max requests and max requests per host to the
same value, to limit the number of new configuration options.

Fixes #22915

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# General
* Allow configuring max requests in native FS for Azure. ({issue}`issuenumber`)

@nineinchnick
Copy link
Member Author

@raunaqmorarka or @ebyhr, I ran tests locally, but please run tests with secrets.

@ebyhr
Copy link
Member

ebyhr commented Aug 5, 2024

/test-with-secrets sha=ad3f2192ea7a58046584a2157acbe4ad3001ce32

@github-actions
Copy link

github-actions bot commented Aug 5, 2024

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10246479126

@nineinchnick nineinchnick force-pushed the azure-max-connections-config branch 2 times, most recently from 1b6d712 to cce712d Compare August 5, 2024 15:08
Copy link
Member

@anusudarsan anusudarsan Aug 26, 2024

Choose a reason for hiding this comment

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

you previously had per-host set to a lower value. does this mean we now need two config options?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have any specific reason for using different values, so I'd rather have one config option.

@github-actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Sep 17, 2024
Max requests where increased from 5 to 2 * number of CPUs in
bd289dc. With a very large number of
nodes in a cluster, this can cause throttling and result in failed
queries. Make it configurable allows to tune the value, if the default
one is too big.

This change also sets the max requests and max requests per host to the
same value, to limit the number of new configuration options.
@nineinchnick nineinchnick force-pushed the azure-max-connections-config branch from cce712d to 8b31b10 Compare September 18, 2024 06:39
@nineinchnick
Copy link
Member Author

@raunaqmorarka @wendigo PTAL and maybe merge this, I forgot about it for a while.

@wendigo
Copy link
Contributor

wendigo commented Sep 18, 2024

Let's merge it!

@wendigo
Copy link
Contributor

wendigo commented Sep 18, 2024

I'm running this with cloud tests

@wendigo wendigo merged commit db35155 into trinodb:master Sep 18, 2024
@nineinchnick nineinchnick deleted the azure-max-connections-config branch September 18, 2024 10:04
@github-actions github-actions bot added this to the 459 milestone Sep 18, 2024
@mosabua
Copy link
Member

mosabua commented Sep 18, 2024

@nineinchnick can you send a docs PR?

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.

Azure throttling issues on version 452 using native fs support

6 participants