Refactor fetcher, polling strategy and manager #236
Conversation
In order to be able to customize the order and manner in which Shoryuken polls SQS queues, the previous hard-coded strategy was extracted into a separate class. This meant that some of the method names were altered to be more generic (for example rebalance_queue_weight! became messages_received), otherwise this was a direct extraction.
Mock "receive" calls appear to use a stronger interpretation of equality. Also altered assertions around raised errors to suppress new warnings.
| expect(subject.instance_variable_get('@queues')).to eq [queue2, queue1] | ||
| it 'calls dispatch_single_messages if worker wants single messages' do | ||
| expect_any_instance_of(described_class).to receive(:dispatch_single_messages) | ||
| .with(queue_config_of(queue)) |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
mariokostelac
force-pushed
the
mario/extract_fetcher
branch
from
d9e13fb to
e69752a
Compare
mariokostelac
changed the title
mariokostelac
force-pushed
the
mario/extract_fetcher
branch
from
434713a to
e69752a
Compare
| rescue => ex | ||
| logger.error { "Error fetching message: #{ex}" } | ||
| logger.error { ex.backtrace.first } | ||
| [] |
There was a problem hiding this comment.
@mariokostelac not sure if we need rescue => ex where, as we are using watchdog wrapping the method body.
There was a problem hiding this comment.
We should use one or another, not both, that's for sure. I do like having error handling here because SQS does return 500 now and then and it'd be nice to return empty array / retry fetch in that case.
There was a problem hiding this comment.
@mariokostelac hm nvm just saw that the current implementation does the same.
|
Hey @mariokostelac it's looking super good. I did an initial PR code review, but I'm going to checkout it and test it locally, so I can understand it better. |
|
@mariokostelac I could perform an end-to-end test today. It seems to be working fine, but |
|
@phstc ok, I will apply changes based on your comments and take a look at Ctrl-C issue. It worked for me (that's how I stopped it), but could be I broke something. |
|
@mariokostelac if |
|
@mariokostelac let's merge it |
🍻 so, we can also merge
|
@phstc I see I have not write rights anymore. Do you prefer creating another PR for that (from forked repo)? |
|
@mariokostelac I think that makes sense. |
|
Closing in favor of #236! |
Simplifying manager, fetcher and polling strategy in order to make whole system easier to understand, test and more performant
I've done some testing locally for next setup
For concurrency set to 1, throughput increased ~50%.
For concurrency set to 10, throughput increased ~30%.
For concurrency set to 20, throughput increased ~20%.
It's important to note that increase becomes higher if jobs gets faster (shorter).
WDYT @phstc?