Avoid memory allocation by reusing TrackerClient in DegraderStrategy#1121
Open
goktuggurler wants to merge 3 commits intolinkedin:masterfrom
Open
Avoid memory allocation by reusing TrackerClient in DegraderStrategy#1121goktuggurler wants to merge 3 commits intolinkedin:masterfrom
goktuggurler wants to merge 3 commits intolinkedin:masterfrom
Conversation
goktuggurler
commented
Oct 22, 2025
|
|
||
| private List<DegraderTrackerClient> castToDegraderTrackerClients(Map<URI, TrackerClient> trackerClients) | ||
| { | ||
| List<DegraderTrackerClient> degraderTrackerClients = new ArrayList<>(trackerClients.size()); |
Author
There was a problem hiding this comment.
Memory allocation followed by O(N) operations.
goktuggurler
commented
Oct 22, 2025
| } | ||
|
|
||
| List<DegraderTrackerClient> degraderTrackerClients = castToDegraderTrackerClients(trackerClients); | ||
| Collection<TrackerClient> trackerClientList = trackerClients.values(); |
Author
There was a problem hiding this comment.
castToDegraderTrackerClients return an empty list if trackerClients cannot be cast to DegradedTrackerClient. This is to mimic that behavior.
goktuggurler
commented
Oct 22, 2025
| if (excludedUris == null) | ||
| { | ||
| excludedUris = new HashSet<>(); | ||
| excludedUris = Collections.emptySet(); |
Author
There was a problem hiding this comment.
Avoid another unnecessary memory allocation.
goktuggurler
commented
Oct 22, 2025
| warn(_log, "Can not find hash ring to use"); | ||
| } | ||
|
|
||
| Map<URI, DegraderTrackerClient> trackerClientMap = new HashMap<>(trackerClients.size()); |
Author
There was a problem hiding this comment.
Another memory allocation followed by O(N) complexity.
goktuggurler
commented
Oct 22, 2025
| } | ||
|
|
||
| private DegraderTrackerClient searchClientFromUri(URI uri, List<DegraderTrackerClient> trackerClients) | ||
| private DegraderTrackerClient searchClientFromUri(URI uri, Map<URI, TrackerClient> trackerClients) |
Author
There was a problem hiding this comment.
When it is a map, we do the search in O(1) time rather than O(N).
| @@ -1049,7 +1022,7 @@ public static void overrideMinCallCount(int partitionId, double newOverrideDropR | |||
| * @return True if we should update, and false otherwise. | |||
| */ | |||
| protected static boolean shouldUpdatePartition(long clusterGenerationId, PartitionDegraderLoadBalancerState partitionState, | |||
Contributor
There was a problem hiding this comment.
Can we create a new method and call the new one from the old? This is a protected method and we'd like backwards compatibility.
Author
There was a problem hiding this comment.
Kept the old one and created the overloaded version of it. The old one does the conversion (inefficient but still accessible).
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
2 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 call from SimpleLoadBalancer to DegraderStrategy passes a
Map<URI, TrackerClient>object for thegetTrackerClient()method to handle load balancing. This object was being converted toList<DegraderTrackerClient>usingcastToDegraderTrackerClients()method. However, during the load balancing operation, this casting operation was not needed. This PR aims to rectify that. The PR also have positive impact ongetRing()method which was suffering from the same process.We have updated the unit tests accordingly.