Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERL: Use IP address instead of IP:port pair #6176

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Nov 20, 2024

Summary

Make ERL more strict by distinguishing IP addresses instead of connections.

Test Plan

Cluster performance test results
image

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 51.88%. Comparing base (db7f162) to head (3991111).

Files with missing lines Patch % Lines
util/rateLimit.go 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6176      +/-   ##
==========================================
+ Coverage   51.86%   51.88%   +0.02%     
==========================================
  Files         639      639              
  Lines       85492    85494       +2     
==========================================
+ Hits        44338    44358      +20     
+ Misses      38339    38322      -17     
+ Partials     2815     2814       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@algorandskiy algorandskiy requested review from cce and gmalouf November 20, 2024 13:47
// guard clauses, and preventing the ElasticRateLimiter from draining its own sharedCapacity
if !exists || q == erl.sharedCapacity {
return
}
delete(erl.capacityByClient, c)
delete(erl.capacityByClient, addr)
Copy link
Contributor

@cce cce Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say you have N conns per IP, ERL sets up the closeReservation(wsPeer) is called by the wsPeer's OnClose() handler for each of N conns (not per IP) so you will end up calling delete multiple times for each conn (not per IP) and also, the capacity that is reserved for this IP (CapacityPerReservation) will be returned N times

Copy link
Contributor

@cce cce Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, since N conns are sharing one erl.capacityByClient[IP] entry, ERL will only call wsPeer.OnClose() when it sees the IP missing from the map. So that means if the first conn goes away, it will return the CapacityPerReservation back and delete itself from the map, even though the other conns are still running.

I think there needs to be an implementation of the ErlClient interface, like a meta-client, that manages setting OnClose() handlers for all the wsPeers sharing the same IP, something like that, and function that takes a wsPeer and looks it up by IP, and returns a IPBasedERLClient impl to pass to ConsumeCapacity(c ErlClient). Then when the IPBasedErlClient notices all the child wsPeers have called OnClose, it calls its own OnClose, which in turns calls ERL.closeReservation()

@algorandskiy algorandskiy marked this pull request as draft November 21, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants