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

Beanstalk support in WPA #69

Merged
merged 50 commits into from
Jun 19, 2020
Merged

Beanstalk support in WPA #69

merged 50 commits into from
Jun 19, 2020

Conversation

alok87
Copy link
Contributor

@alok87 alok87 commented May 12, 2020

Moved from #57

Changes made on top of #57

  • Fixed the concurrent write issue, maps cannot be used for concurrent writes so replaced the impl with the sync.Map.
  • Changed the beanstalk library with a well maintained library.
  • Removed the sqs changes.
  • Made the beanstalk implementation exactly same as sqs. The poll function is very much same for both of them now.
  • Fixed the bug in which specifying both beanstalk and sqs starts polling all queues by filtering the queues for each poller.
  • Added 40.7% test coverage for the queue package (beanstalk tests).
    ok github.com/practo/k8s-worker-pod-autoscaler/pkg/queue 43.028s coverage: 41.3% of statements
    • Encapsulated and mocked the beanstalk client for testing the main poll and sync logic.
    • Used a local beanstalk process to test the beanstalk client stats and longPoll calls.

@sushant-115

sushant-115 and others added 30 commits March 4, 2020 16:18
Old format: bs|test-tube|beanstalkd|11300
New format: beanstalk://beanstalkd:11300/test-tube
Using standard url format for beanstalk queue uri
Old format: bs|test-tube|beanstalkd|11300
New format: beanstalk://beanstalkd:11300/test-tube
Old format: bs|test-tube|beanstalkd|11300
New format: beanstalk://beanstalkd:11300/test-tube
Copy link
Contributor Author

@alok87 alok87 left a comment

Choose a reason for hiding this comment

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

UPDATE: fixed in 8153761

There is a concurrent write issue, that needs to be fixed.

fatal error: concurrent map read and map write

goroutine 2493 [running]:
runtime.throw(0x1679af0, 0x21)
    /usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc001224c28 sp=0xc001224bf8 pc=0x433912
runtime.mapaccess1_faststr(0x149e320, 0xc000243290, 0xc0009a6a00, 0x48, 0x460010)
    /usr/local/go/src/runtime/map_faststr.go:21 +0x43c fp=0xc001224c98 sp=0xc001224c28 pc=0x4129cc
github.com/practo/k8s-worker-pod-autoscaler/pkg/queue.(*Beanstalk).getBeanstalkClient(0xc000131e80, 0xc0009a6a00, 0x48, 0xc0003a170e, 0x2, 0x2)
    /src/pkg/queue/beanstalk.go:48 +0x59 fp=0xc001224d10 sp=0xc001224c98 pc=0x134fb49
github.com/practo/k8s-worker-pod-autoscaler/pkg/queue.(*Beanstalk).receiveQueueLength(0xc000131e80, 0xc0009a6a00, 0x48, 0x5c0000000040505d, 0xc001224dc8, 0xc001224dc8)
    /src/pkg/queue/beanstalk.go:72 +0x43 fp=0xc001224d70 sp=0xc001224d10 pc=0x134fde3
github.com/practo/k8s-worker-pod-autoscaler/pkg/queue.(*Beanstalk).poll(0xc000131e80, 0xc0025e2720, 0x2f, 0xc0009a6a21, 0x27, 0xc000d3f680, 0x9, 0xc0009a6a00, 0x48, 0xc0009a6a0c, ...)
    /src/pkg/queue/beanstalk.go:138 +0x49 fp=0xc001224dd8 sp=0xc001224d70 pc=0x1350329
github.com/practo/k8s-worker-pod-autoscaler/pkg/queue.(*Poller).runPollThread(0xc0002433e0, 0xc0025e2720, 0x2f)
    /src/pkg/queue/poller.go:46 +0x88 fp=0xc001224fc8 sp=0xc001224dd8 pc=0x13504e8
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc001224fd0 sp=0xc001224fc8 pc=0x4634e1
created by github.com/practo/k8s-worker-pod-autoscaler/pkg/queue.(*Poller).Run
    /src/pkg/queue/poller.go:93 +0x1db

alok87 and others added 3 commits June 12, 2020 10:23
map only supports concurrent reads, but not concurrent read and write.
sync.Map was used to solve this which supports this.

https://golang.org/doc/faq#atomic_maps
https://golang.org/pkg/sync/
#69 (review)
Any changes made to sqs should not go with this PR
@alok87 alok87 changed the title Feature/Beanstalk Queuing Service Beanstalk support in WPA Jun 13, 2020
@alok87 alok87 force-pushed the beanstalk branch 2 times, most recently from 79434d6 to ff981fb Compare June 15, 2020 08:46
Reason: Pave the way for unit tests
alok87 added 3 commits June 15, 2020 17:20
Beanstalk queues are only present when there are jobs. This behaviour is one of the behaviours of beanstalk which is not same as SQS.
Queue: Beanstalk tests
This covers most of the beanstalk testing scenarios
@alok87 alok87 requested a review from justjkk June 17, 2020 11:56
@alok87 alok87 added the enhancement New feature or request label Jun 17, 2020
@alok87
Copy link
Contributor Author

alok87 commented Jun 18, 2020

Found a bug: when --queue-services=sqs,beanstalk the beanstalk poller also starts operating on the sqs ones. This should not happen.

Fixed in 2f6a0c9

SQS poller should operate on only sqs WPA resources and same goes for beanstalk.
Also change the default to start both.
#69 (comment)
@alok87 alok87 merged commit 5c7c0a6 into master Jun 19, 2020
@alok87 alok87 mentioned this pull request Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants