Fail query with large taskUpdate size#13584
Merged
wenleix merged 1 commit intoprestodb:masterfrom Oct 24, 2019
Merged
Conversation
wenleix
reviewed
Oct 22, 2019
presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/remotetask/HttpRemoteTask.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/remotetask/HttpRemoteTask.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/remotetask/HttpRemoteTask.java
Outdated
Show resolved
Hide resolved
NikhilCollooru
force-pushed
the
updateRequestSize
branch
from
d47d56e to
d2306b8
Compare
Contributor
|
An not-so-related note from a shadow production test: Status fetch: 917M call The ratio can change depends on workload, but the magnitude should be more or less similar. If we normalize to have 1 plan serialize:
|
NikhilCollooru
force-pushed
the
updateRequestSize
branch
from
d2306b8 to
841da3d
Compare
NikhilCollooru
changed the title
wenleix
approved these changes
Oct 23, 2019
Contributor
wenleix
left a comment
wenleix
left a comment
There was a problem hiding this comment.
LGTM % nits.
Also note new added configuration property always requires release notes. See an example in https://prestodb.github.io/docs/current/release/release-0.225.html :
Add configuration property experimental.max-total-running-task-count and experimental.max-query-running-task-count to limit number of running tasks ......
presto-main/src/main/java/com/facebook/presto/server/remotetask/HttpRemoteTaskFactory.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/InternalCommunicationConfig.java
Outdated
Show resolved
Hide resolved
NikhilCollooru
force-pushed
the
updateRequestSize
branch
3 times, most recently
from
11e4c04 to
606a58e
Compare
The max taskUpdate size can be set using a new parameter experimental.internal-communication.max-task-update-size. Also add changes to monitor the size of taskUpdates that do not have the plan info. For this we sample 1 in 100 to avoid concurrency problems.
NikhilCollooru
force-pushed
the
updateRequestSize
branch
from
606a58e to
6f94a77
Compare
This was referenced Nov 11, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The max taskUpdate size can be set using a new parameter
experimental.internal-communication.max-task-update-size.
Also add code changes to monitor the size of taskUpdates
that do not have the plan info. For this we sample
1 in 100 to avoid concurrency problems.