[router] Reduced allocations in router read path #1367
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
A lot of multi-key specific state was moved out of VenicePath and into its subclasses, which makes single get queries require less heap space. This includes streaming (chunkedResponse), HAR (helixGroupId and requestId), longTailRetryThresholdMs and responseHeaders. In addition, all kinds of VenicePaths are also getting their heap size reduced by leveraging shared instances where possible. For example: - The resourceName, storeName and versionNumber properties are replaced by a single reference to a shared instance of StoreVersionName, which is a new class obtained from a NameRepository (same idea as the PubSubTopicRepository). - The smartLongTailRetryEnabled, smartLongTailRetryAbortThresholdMs and longTailRetryThresholdMs properties are replaced by a single pointer to a RouterRetryConfig object, which is a simple facade wrapping the VeniceRouterConfig. - The responseDecompressor is now coming from a map of shared instances in the VenicePathParser. Miscellaneous: - Various refactorings enable the VenicePath constructors to have fewer params and to make more of the properties final. - Made as many properties as possible final in the VeniceRouterConfig. - Minor tweaks to the main README page.
…uilder. Deleted VeniceMetricsProvider which can be trivially inlined.
by tests to inject a MockTime, it is done from a subclass, and therefore can be achieved differently via extension, rather than composition.
lusong64
reviewed
Dec 13, 2024
| final BiConsumer<AggRouterHttpRequestStats, String> statsRecorder; | ||
|
|
||
| FailureType(BiConsumer<AggRouterHttpRequestStats, String> statsRecorder) { | ||
| this.reportUnhealthy = statsRecorder == null; |
There was a problem hiding this comment.
So the entire reportUnhealthy is used to recorded whether we have a non-null BiConsumer? Would it possible to just use
this.statsRecorder = Optional.ofNullable(statsRecorder)
.orElse((stats, store) -> {});
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.
A lot of multi-key specific state was moved out of VenicePath and into its subclasses, which makes single get queries require less heap space. This includes streaming (chunkedResponse), HAR (helixGroupId and requestId), longTailRetryThresholdMs and responseHeaders.
In addition, all kinds of VenicePaths are also getting their heap size reduced by leveraging shared instances where possible. For example:
The resourceName, storeName and versionNumber properties are replaced by a single reference to a shared instance of StoreVersionName, which is a new class obtained from a NameRepository (same idea as the PubSubTopicRepository).
The smartLongTailRetryEnabled, smartLongTailRetryAbortThresholdMs and longTailRetryThresholdMs properties are replaced by a single pointer to a RouterRetryConfig object, which is a simple facade wrapping the VeniceRouterConfig.
The responseDecompressor is now coming from a map of shared instances in the VenicePathParser.
Miscellaneous:
Various refactorings enable the VenicePath constructors to have fewer params and to make more of the properties final.
Made as many properties as possible final in the VeniceRouterConfig.
Minor tweaks to the main README page.
How was this PR tested?
CI.
Does this PR introduce any user-facing changes?