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.

Sign up for GitHub

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

Jump to bottom

Refactor load balancer, decouple it from fadeIn #2381

Closed
wants to merge 6 commits into from
Closed

Refactor load balancer, decouple it from fadeIn #2381

wants to merge 6 commits into from

Conversation

RomanZavodskikh
Copy link
Contributor

@RomanZavodskikh RomanZavodskikh commented Jun 8, 2023 •
edited
Loading

Updates #2346

  • Added sync.Mutex to routing.Route
  • Created HealthyEndpoint* fields in routing.Routing
  • Refactor load balancer, decouple it from fadeIn
  • Updated the code for CI tests to work
  • Fixed the searchRing function for consistentHash

@RomanZavodskikh RomanZavodskikh force-pushed the refactorLB branch 2 times, most recently from 68acf23 to d093de2 Compare June 9, 2023 16:44
@szuecs szuecs changed the title Refactror load balancer, decouple it from fadeIn Refactor load balancer, decouple it from fadeIn Jun 9, 2023
RomanZavodskikh
RomanZavodskikh commented Jun 12, 2023
View reviewed changes
@RomanZavodskikh
Copy link
Contributor Author

Added some code trying to avoid races.

szuecs
szuecs reviewed Jun 12, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 12, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 12, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 12, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 12, 2023
View reviewed changes
@RomanZavodskikh
Copy link
Contributor Author

Addressed some of Sandor's comments. However, still searching for the good place for go rt.goodEndpointerGoroutine().

@szuecs
Copy link
Member

szuecs commented Jun 12, 2023

@RomanZavodskikh I think the only place to start a goroutine is the startReceivingUpdates() func in routing.go.

@RomanZavodskikh
Copy link
Contributor Author

Decided not to close routeTable object in the startReceivingUpdates goroutine. Instead I decided to create quit channel for goodEndpointerGoroutine instead.

@RomanZavodskikh
Copy link
Contributor Author

Made consistentHash.Apply work in O(1) time.

RomanZavodskikh
RomanZavodskikh commented Jun 15, 2023
View reviewed changes
@RomanZavodskikh
Copy link
Contributor Author

Moved ctx.Route.Lock() lower.

@RomanZavodskikh RomanZavodskikh force-pushed the refactorLB branch 2 times, most recently from ddcc4b5 to c41548a Compare June 15, 2023 20:36
@RomanZavodskikh RomanZavodskikh marked this pull request as ready for review June 16, 2023 07:52
RomanZavodskikh
RomanZavodskikh commented Jun 16, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 16, 2023
View reviewed changes
szuecs
szuecs reviewed Jun 16, 2023
View reviewed changes
@szuecs
Copy link
Member

szuecs commented Jun 16, 2023

The code looks good to me some worries, but it's more feeling than anything else, so data will convince me.

@RomanZavodskikh RomanZavodskikh force-pushed the refactorLB branch 10 times, most recently from b80d7be to 652fe65 Compare July 12, 2023 13:40
RomanZavodskikh
RomanZavodskikh commented Jul 13, 2023
View reviewed changes
routing/routing.go
r.mx.Lock()
defer r.mx.Unlock()

if len(r.HealthyEndpoints) == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's initialize in constructor/updateHealthyEndpoints.

RomanZavodskikh
RomanZavodskikh commented Jul 13, 2023
View reviewed changes
loadbalancer/algorithm.go Outdated
}
r.oldEpsLen = int64(len(endpoints))
r.index = (r.index + 1) % int64(len(endpoints))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to store r.index here.

loadbalancer/fadein_test.go Outdated
updateCh := make(chan time.Time)
defer close(updateCh)
r := routing.New(routing.Options{UpdateGoodEndpointsChan: updateCh})
r.SetRoute(ctx.Route)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to create dataclient, preprocessors, postprocessors, etc...

routing/datasource.go
}

ri.mx.Lock()
ri.HealthyEndpoints = newHealthyEndpoints
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens during routing table update?

Roman Zavodskikh added 6 commits August 16, 2023 18:18
Added sync.Mutex to routing.Route
912ad5a 
Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
Created HealthyEndpoint* fields in routing.Routing
a06709e 
Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
Temporarily disable load test between old endpoints
ee26f52
Refactor load balancer, decouple it from fadeIn
b03aaf2 
Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
Updated the code for CI tests to work
015b62f 
Updates #2346

Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
Added fadein for powerOfRandomNChoices as well
8d8ad81 
Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
This was referenced Aug 17, 2023
Closed
Merged
@RomanZavodskikh
Copy link
Contributor Author

Will do this work here #2634 .
Here unfortunately I've taken too much work I should've split it before creating this huge PR. So I did in #2634 and other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szuecs szuecs szuecs left review comments

@AlexanderYastrebov AlexanderYastrebov AlexanderYastrebov left review comments

@MustafaSaber MustafaSaber MustafaSaber left review comments

Assignees

@AlexanderYastrebov AlexanderYastrebov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RomanZavodskikh @szuecs @AlexanderYastrebov @MustafaSaber