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

chore: make cslb configurable #5451

Draft
wants to merge 2 commits into
base: release/1.41.x
Choose a base branch
from
Draft

Conversation

cisse21
Copy link
Member

@cisse21 cisse21 commented Jan 22, 2025

Description

  • Add configurability options to CSLB Picker
  1. NewPowerOfTwo
  2. NewRoundRobin
  3. NewLeastLoadedRandom
  4. NewLeastLoadedRoundRobin
  • Add configurability options to CSLB Health Check

Linear Ticket

Fixes PIPE-1860

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@cisse21 cisse21 force-pushed the chore.configurableCSLB branch from ed55f43 to d06c393 Compare January 22, 2025 03:12
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 74.73%. Comparing base (a679205) to head (2bcb2c6).

Files with missing lines Patch % Lines
processor/transformer/transformer.go 0.00% 29 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           release/1.41.x    #5451      +/-   ##
==================================================
- Coverage           74.81%   74.73%   -0.09%     
==================================================
  Files                 440      440              
  Lines               61670    61699      +29     
==================================================
- Hits                46140    46108      -32     
- Misses              12993    13045      +52     
- Partials             2537     2546       +9     

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

@cisse21 cisse21 force-pushed the chore.configurableCSLB branch 3 times, most recently from cb0224b to 97cc532 Compare January 22, 2025 06:52
@cisse21 cisse21 changed the base branch from master to release/1.41.x January 22, 2025 08:26
@cisse21 cisse21 marked this pull request as ready for review January 22, 2025 08:27
@cisse21 cisse21 requested review from lvrach and Sidddddarth January 22, 2025 08:27
case "polling":
return health.NewPollingChecker(
health.PollingCheckerConfig{},
health.NewSimpleProber(trans.config.userTransformationURL),
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't using trans.config.userTransformationURL adversely affect destination transformation considering the same client is used..?

Copy link
Member

Choose a reason for hiding this comment

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

I guess there's no adverse effects, but we're not probing the right endpoint

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be fixing this in the new separate clients PR

@@ -310,6 +315,39 @@ func (t *HTTPLBTransport) NewRoundTripper(scheme, target string, config httplb.T
return httplb.RoundTripperResult{RoundTripper: t.Transport, Close: t.CloseIdleConnections}
}

func (trans *handle) getPicker() func(prev picker.Picker, allConns conn.Conns) picker.Picker {
Copy link
Member

Choose a reason for hiding this comment

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

we are adding method that contain logic specific to httplb, to a generic component.

It would be preferable to encapsulate this in a separate component. A simple wrapper on httplb.NewClient would be enough.

Something like

HTTPLBFromConfig(c *config.Config) httplb.Client {
pickerType := config.GetString("Transformer.Client.httplb.pickerType", "power_of_two")
...

return httplb.NewClient(....
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done... is this behaviour what you expected?

@cisse21 cisse21 force-pushed the chore.configurableCSLB branch from 97cc532 to 2bcb2c6 Compare January 22, 2025 14:05
@cisse21 cisse21 requested review from Sidddddarth and lvrach January 22, 2025 14: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.

4 participants