[router] Reduced allocations in router read path #1367
Merged
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
lusong64
reviewed
Dec 13, 2024
...ice-router/src/main/java/com/linkedin/venice/router/api/RouterExceptionAndTrackingUtils.java
Outdated
Show resolved
Hide resolved
FelixGV
force-pushed
the
router_allocation_cleanup
branch
from
3c28320 to
ccdca3e
Compare
2 tasks
|
Carved out a portion of this big-ish PR into this other fairly trivial PR: #1405. |
FelixGV
force-pushed
the
router_allocation_cleanup
branch
from
ccdca3e to
9a5d502
Compare
|
Rebased after merging #1405 to reduce the PR size, but also added a flaky test fix which still increases it a bit more 😅 ... oh well 🤷 |
2 tasks
|
Will be able to further reduce the size of this PR after rebasing on top of #1413. EDIT: Done. |
FelixGV
force-pushed
the
router_allocation_cleanup
branch
from
9a5d502 to
bbc9f53
Compare
2 tasks
FelixGV
force-pushed
the
router_allocation_cleanup
branch
3 times, most recently
from
d1bd342 to
85a52c5
Compare
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. - The smartLongTailRetryEnabled, smartLongTailRetryAbortThresholdMs and longTailRetryThresholdMs properties are replaced by a single pointer to a RouterRetryConfig object, which is a simple facade wrapping the VeniceRouterConfig. - Removed the time property from VenicePath since the only time it is used by tests to inject a MockTime, it is done from a subclass, and therefore can be achieved via extension, rather than composition. - The responseDecompressor is now coming from a map of shared instances in the VenicePathParser. New config: - name.repository.max.entry.count : controls the maximum number of entries (per type) to be cached in the NameRepository class. For now this config is used only in the router, but it would likely become used in the server and controller as well, later on. Miscellaneous: - Various refactorings enable the VenicePath constructors to have fewer params and to make more of the properties final. - Made use of StoreName rather than String in the VenicePathParser's RetryManager maps. - Added a default IntSet getVersionNumbers function to the Store interface. This is a convenience function since oftentimes we only want the version numbers, and not the complete Version objects. - Cleaned up generics in the VenicePathParser and ScatterGatherHelper builder. - Deleted VeniceMetricsProvider which can be trivially inlined.
FelixGV
force-pushed
the
router_allocation_cleanup
branch
from
85a52c5 to
d647969
Compare
|
Rebased |
ZacAttack
reviewed
Jan 21, 2025
services/venice-router/src/main/java/com/linkedin/venice/router/api/VenicePartitionFinder.java
Show resolved
Hide resolved
ZacAttack
reviewed
Jan 21, 2025
...ces/venice-router/src/main/java/com/linkedin/venice/router/api/path/VeniceSingleGetPath.java
Show resolved
Hide resolved
ZacAttack
reviewed
Jan 21, 2025
services/venice-router/src/main/java/com/linkedin/venice/router/api/path/VenicePath.java
Show resolved
Hide resolved
ZacAttack
reviewed
Jan 21, 2025
services/venice-router/src/main/java/com/linkedin/venice/router/api/VenicePathParser.java
Show resolved
Hide resolved
ZacAttack
approved these changes
Jan 22, 2025
|
Thanks for the review! |
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.
The smartLongTailRetryEnabled, smartLongTailRetryAbortThresholdMs and longTailRetryThresholdMs properties are replaced by a single pointer to a RouterRetryConfig object, which is a simple facade wrapping the VeniceRouterConfig.
Removed the time property from VenicePath since the only time it is used by tests to inject a MockTime, it is done from a subclass, and therefore can be achieved via extension, rather than composition.
The responseDecompressor is now coming from a map of shared instances in the VenicePathParser.
New config:
Miscellaneous:
Various refactorings enable the VenicePath constructors to have fewer params and to make more of the properties final.
Made use of StoreName rather than String in the VenicePathParser's RetryManager maps.
Added a default IntSet getVersionNumbers function to the Store interface. This is a convenience function since oftentimes we only want the version numbers, and not the complete Version objects.
Cleaned up generics in the VenicePathParser and ScatterGatherHelper builder.
Deleted VeniceMetricsProvider which can be trivially inlined.
How was this PR tested?
CI.
Does this PR introduce any user-facing changes?