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

Add support for per-target options #6

Merged
merged 4 commits into from
Jun 5, 2023
Merged

Add support for per-target options #6

merged 4 commits into from
Jun 5, 2023

Conversation

jhump
Copy link
Member

@jhump jhump commented Jun 3, 2023

This also refactors how "keep warm" targets are determined: basically, any explicitly configured backend target is kept warm. This also adds a new option, WithDisallowUnconfiguredTargets, which means the client will refuse to service requests to unknown backend targets.

When moving things around to handle the fact that many options are now per-target, I realized the applyRequestTimeout I had added was never called. When I actually revised the code to use it, I've encountered a conundrum: I've got a cancel function, but don't know when to call it. To prevent resource leaks, it should be called when the operation completes (to release the timer goroutine that is otherwise scheduled to cancel the context if it takes too long). So... I added a rudimentary/preliminary form of "detecting when the request is done", and went ahead and also use that to track the number of active requests per leaf transport (which we'll need for least-loaded load balancing anyway). That makes this the start of TCN-1867.

Resolves TCN-1878

@linear
Copy link

linear bot commented Jun 3, 2023

TCN-1878 Add ability to configure some options on a per-backend basis

In addition to defining "defaults" (when configuration is uniform), we'll need a way to configure many of the options on a per-backend basis. A good option is to provide a second kind of option like BackendOption (in addition to ClientOption), and the options that can vary per-backend can implement that interface, too. That way they can be provided as ClientOption and used as a global/default, but also used in something like the following, allowing users to per-backend overrides:

func WithBackendOptions(scheme, hostPort string, opts ...BackendOption) ClientOption {
    // implement me
}

Using WithBackendOptions could also automatically add the given scheme and host:port to the set of "keep warm" targets, too.

@jhump jhump requested a review from jchadwick-buf June 3, 2023 05:21
Copy link
Member

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

A few comments, most importantly a concern about how to overwrite Response.Body.

transport.go Outdated Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
transport.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
Base automatically changed from jh/close to main June 5, 2023 16:15
Copy link
Member

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

Yeah, that seems fine too. A little verbose, but not so bad.

@jhump jhump merged commit c83a0cf into main Jun 5, 2023
@jhump jhump deleted the jh/target-options branch June 5, 2023 16:22
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.

2 participants