search

openfaas / of-watchdog

Reverse proxy for STDIO and HTTP microservices
MIT License
259 stars 115 forks source link

Add Concurrency Limiter #54

Closed sargun closed 5 years ago

sargun commented 5 years ago

Description

This enables limiting concurrency. It is a naive approach which will reject requests as soon as they exceed the maximum number of in-flight requests.

Motivation and Context

How Has This Been Tested?

There are tests in the code that test for all of the obvious possible variants. The code is written in such a way that testing is easy.

Types of changes

Checklist:

alexellis commented 5 years ago

Hi @sargun thanks for raising this.

Are there any differences from the original PR?

Alex

alexellis commented 5 years ago

One of the comments/questions I left on the issue was:

Given that this is required in the watchdog, what is an appropriate way to handle the code in both places? A shared package or duplicated code figured out later?

sargun commented 5 years ago

Some changes:

  1. Broke out the concurrency-limiter into its own code. This will allow us to use this code in the legacy handler. I do not see a way to vendor / gomod code in the repo, but I'm willing to hand-vendor, use go mod, copy the directory, or govendor.
  2. Renamed concurrency_limit to max_inflight
  3. Normalized usage between http.Handler, and http.HandlerFunc
alexellis commented 5 years ago

In this part of the PR unit testing and manual e2e validation is normally mentioned.

How Has This Been Tested?

I'm guessing that you've been using a HTTP-template of your own or from the template store and that's how the original PR got raised here instead of on the classic watchdog. Which templates have you tested with, in a Docker image, deployed to an OpenFaaS cluster and in http mode?

alexellis commented 5 years ago

Thanks for adding your review @LucasRoesler

@sargun this looks close to being merged, at which point I will cut a binary release for you.

Once ready there will also be a quick rebase needed with origin/master. Please avoid doing a merge commit into your branch. I usually do this with git pull origin master --rebase then fix up any conflicts if I see any.

sargun commented 5 years ago

@alexellis PTAL

alexellis commented 5 years ago

Thank you @sargun - please let us know on Slack if there's anything else that we can do to help enable your use-case.

@LucasRoesler thanks for the review :+1:

Here's the release which contains this change:

https://github.com/openfaas-incubator/of-watchdog/releases/tag/0.5.1

A binary is added by the CI process a few minutes after cutting the release.

Follow-up actions

@sargun are you able to run with these follow-up items?

@LucasRoesler do you see anything else that we need to do?

A short / brief guest blog post may be a good follow-up on openfaas.com - I guess we would want to promote this with the use of an IngressController that does automatic retries when it gets a non 2xx code such as Heptio Contour? Problem statement with example / brief description of solution / draw.io conceptual diagram / example to deploy with new flag / conclusion.

Alex

sargun commented 5 years ago

Got it.

sargun commented 5 years ago

@LucasRoesler / @alexellis One quick thing. To complete the backport, does it make sense to move this into the classic watchdog, and then we vendor the classic watchdog? I noticed you just introduced the first use of vendoring.

P.S. Any reason why you didn't use go modules. From my understanding, that's the Golang "Standard" now.

burtonr commented 5 years ago

Hey @sargun,

Thanks for the PR! :100:

The 2 watchdogs are different enough I don't think it makes sense to vendor one into the other. This would likely cause confusion in the future. (in my opinion)

In reference to your question of using go modules, we plan on sticking with dep for the foreseeable future due to the amount of effort involved. You can read a little more about it in the freshly updated contribution guide