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

group client config requests #1395

Merged
merged 63 commits into from
Nov 22, 2024
Merged

group client config requests #1395

merged 63 commits into from
Nov 22, 2024

Conversation

garmr-ulfr
Copy link
Contributor

@garmr-ulfr garmr-ulfr commented Jul 16, 2024

This groups the proxy, geolocation, and user config requests to reduce start up time. The config request service utilizes the callRandomly code from bypass that adds jitter between requests to prevent the thundering herd problem, as well as avoid creating a pattern that censors could detect.

@garmr-ulfr
Copy link
Contributor Author

There are still a couple of things that need to be done, such as update the original config fetcher to not fetch proxy configs, have flashlight start the new config and bypass services, add tests, etc.

@garmr-ulfr garmr-ulfr requested a review from hwh33 July 16, 2024 20:04
Copy link
Contributor

@hwh33 hwh33 left a comment

Choose a reason for hiding this comment

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

Nice work so far @garmr-ulfr!

The config request service utilizes the callRandomly code from bypass that adds jitter between requests to avoid creating a pattern that censors could detect.

Just FYI, another reason is to prevent the thundering herd problem. Probably the more important reason IMO.

apipb/legacy.go Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
config/initializer.go Outdated Show resolved Hide resolved
config/initializer.go Outdated Show resolved Hide resolved
config/proxy/proxy.go Outdated Show resolved Hide resolved
services/config.go Outdated Show resolved Hide resolved
services/config.go Outdated Show resolved Hide resolved
services/http.go Outdated Show resolved Hide resolved
services/http.go Outdated Show resolved Hide resolved
services/http.go Outdated
sleepVal := resp.Header.Get(common.SleepHeader)
if sleepVal != "" {
if sleepTime, err = strconv.ParseInt(sleepVal, 10, 64); err != nil {
logger.Errorf("Could not parse sleep val: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's set a default sleep time in this case. How's one hour sound to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sleepVal here is extra sleep time. If we wanted an hour between polls, wouldn't that be PollInterval?

@hwh33
Copy link
Contributor

hwh33 commented Aug 2, 2024

Just FYI, another reason is to prevent the thundering herd problem. Probably the more important reason IMO.

On that note, if we're generalizing the fetching code, I think we should add exponential back-off on error. Let's max that out at 10 minutes (not including jitter).

@coveralls
Copy link

Coverage Status

coverage: 34.504% (-3.1%) from 37.559%
when pulling 6b79958 on group-client-reqs
into 2d5e5af on main.

@garmr-ulfr garmr-ulfr marked this pull request as ready for review August 14, 2024 19:21
@garmr-ulfr garmr-ulfr requested a review from hwh33 August 14, 2024 19:21
Copy link
Contributor

@WendelHime WendelHime left a comment

Choose a reason for hiding this comment

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

The PR looks really good! I've added a small optional suggestion because I'm allergic to goto and labels but it can be ignored!

services/http.go Outdated Show resolved Hide resolved
geolookup/geolookup.go Outdated Show resolved Hide resolved
flashlight.go Outdated Show resolved Hide resolved
config/proxy/proxy.go Outdated Show resolved Hide resolved
config/proxy/proxy.go Outdated Show resolved Hide resolved
config/proxy/proxy.go Outdated Show resolved Hide resolved
config/proxy/proxy_test.go Outdated Show resolved Hide resolved
flashlight.go Outdated Show resolved Hide resolved
flashlight.go Outdated Show resolved Hide resolved
services/config.go Outdated Show resolved Hide resolved
services/config.go Outdated Show resolved Hide resolved
services/config.go Show resolved Hide resolved
services/http.go Outdated Show resolved Hide resolved
services/http.go Outdated Show resolved Hide resolved
@garmr-ulfr garmr-ulfr requested a review from hwh33 August 19, 2024 15:34
Copy link
Contributor

@hwh33 hwh33 left a comment

Choose a reason for hiding this comment

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

LGTM. Awesome work @garmr-ulfr!

Let's open a PR into lantern-client with these changes. We'll try to get a build out and prioritize thorough testing.

@garmr-ulfr garmr-ulfr removed the request for review from Jovis7 October 22, 2024 21:09
@garmr-ulfr
Copy link
Contributor Author

There is bug causes the fetch to fail when starting Lantern and there are existing proxy configs with IPs that have rotated out.

@garmr-ulfr
Copy link
Contributor Author

garmr-ulfr commented Nov 14, 2024

There is bug causes the fetch to fail when starting Lantern and there are existing proxy configs with IPs that have rotated out.

Fixed.

@garmr-ulfr garmr-ulfr merged commit b41a819 into main Nov 22, 2024
1 of 3 checks passed
@garmr-ulfr garmr-ulfr deleted the group-client-reqs branch November 22, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants