-
Notifications
You must be signed in to change notification settings - Fork 86
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
TT-10676 Upgrade Resurface Pump backend #731
Conversation
Hey @monrax, thanks for the PR! we will have a look! |
Hey @monrax! Could you please provide us with a Resurface license so we can completely test it? |
Hey @tbuchaillot ! Apologies I missed your early messages here as well, my notification tray was a mess. Nice to meet you @mativm02 --I just sent the license to Tom, but if it makes things easier I can send it to you as well. Is [email protected] your current mail address? |
bdc1c64
to
6c24467
Compare
6c24467
to
888d6b5
Compare
Hi all, I just squashed and rebased all commits onto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! thank you @monrax
rp.log.Info(rp.GetName() + " Initialized") | ||
return nil | ||
} | ||
|
||
func (rp *ResurfacePump) initWorker() { | ||
rp.data = make(chan []interface{}, 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the channel size is hardcoded to 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it would be 1 instead of 5. After some load testing I found that having a couple extra slots to avoid blocking right away wouldn't hurt (plus some more just to be safe). I do agree it would be better to make it configurable by defining a new parameter for this pump backend. Please, let me know if this is something you need and I can make a new PR with the required changes. Thanks!
Upgrade the Resurface backend for the Tyk Pump. This upgrade makes the data writing process asynchronous through the use of bounded channels. In addition, the
Shutdown
method has been defined to support graceful shutdown.Description
logger-go
dependency to version 3.3.1, which includes improvements in goroutine management, as well as a newStop
method for graceful shutdown.Shutdown
method for graceful shutdown ofResurfacePump
backend.Related Issue
Issues #729 and #730
Motivation and Context
Stop
method for graceful shutdown of the data capture process.WriteData
might take a long time to return (e.g. longer than the configured redis TTL). This can be a bottleneck that can affect the entire data capture process.How This Has Been Tested
docker-compose.yml
file:pump.conf
file to configure the Resurface pump backend without any timeout:/image/png
endpoint aiming for >4000 RPS (Configurations used: Locust: 2 workers, 50 spawn rate, and 6000 users; Bombardier: concurrency level of 100 with 500k requests)Screenshots (if appropriate)
Types of changes
Checklist
fork, don't request your
master
!master
branch (left side). Also, you should startyour branch off our latest
master
.go mod tidy && go mod vendor
go fmt -s
go vet