add maxBytesInMemory and maxClientResponseBytes to SamplerConfig #12947
Conversation
This file contains 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
| @JsonProperty("timeoutMs") Integer timeoutMs | ||
| @JsonProperty("numRows") @Nullable Integer numRows, | ||
| @JsonProperty("timeoutMs") @Nullable Integer timeoutMs, | ||
| @JsonProperty("maxBytesInMemory") @Nullable Long maxBytesInMemory, |
There was a problem hiding this comment.
Suggested change
| @JsonProperty("maxBytesInMemory") @Nullable Long maxBytesInMemory, | |
| @JsonProperty("maxBytesInMemory") @Nullable HumanReadableBytes maxBytesInMemory, |
| @JsonProperty("numRows") @Nullable Integer numRows, | ||
| @JsonProperty("timeoutMs") @Nullable Integer timeoutMs, | ||
| @JsonProperty("maxBytesInMemory") @Nullable Long maxBytesInMemory, | ||
| @JsonProperty("maxClientResponseBytes") @Nullable Long maxClientResponseBytes |
There was a problem hiding this comment.
Suggested change
| @JsonProperty("maxClientResponseBytes") @Nullable Long maxClientResponseBytes | |
| @JsonProperty("maxClientResponseBytes") @Nullable HumanReadableBytes maxClientResponseBytes |
There was a problem hiding this comment.
yeah, that seems reasonable, forgot about that thing 👍
| @@ -253,6 +253,7 @@ public void applyDataSegmentChangeRequests( | |||
|
|
|||
| final AsyncContext asyncContext = req.startAsync(); | |||
|
|
|||
|
|
|||
| asyncContext.addListener( | |||
FrankChen021
approved these changes
Aug 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Adds additional limits
maxBytesInMemoryandmaxClientResponseBytesto the Druid sampler API config, to augment existing options ofnumRowsandtimeoutMs. While these two configs are quite similar,maxBytesInMemoryis oriented around controlling how big theIncrementalIndexwhich performs the sampling will be, whilemaxClientResponseBytesis an attempt to allow clients to specify the maximum size of response they would prefer to handle, which might vary significantly frommaxBytesInMemory.maxClientResponseBytesis done in a rather sad way of serializing (and throwing away) the sampler rows and counting the bytes. In the future we might want to rework the sampler API to be a streaming API and do the counting of this stuff and enforcement of limits a bit more naturally.This PR has: