Make fetcher pause interval configurable #214

darraghotoole · 2016-05-25T12:57:59Z

The fetcher currently pauses for one second when all processes are busy. When running in single-threaded mode this effectively means we can only process one job per second. This PR allows us to configure the pause interval (eg: 0.1 for 100ms) instead of using the hardcoded value.

houndci-bot · 2016-05-25T12:58:03Z

spec/shoryuken/manager_spec.rb

+      expect { Shoryuken::Manager.new(nil) }
+        .to raise_error(ArgumentError, 'Dispatch interval value -1 is invalid, it needs to be a positive number')
+    end
+


Extra empty line detected at block body end.

houndci-bot · 2016-05-25T14:14:17Z

lib/shoryuken/manager.rb

+      end
+
+      unless @dispatch_interval > 0
+        raise(ArgumentError, "Dispatch interval value #{@dispatch_interval} is invalid, it needs to be a positive number")


Line is too long. [122/120]

houndci-bot · 2016-05-26T10:04:30Z

lib/shoryuken/manager.rb

+      end
+
+      unless @fetcher_pause_interval > 0
+        raise(ArgumentError, "Fetcher pause interval value #{@fetcher_pause_interval} is invalid, it needs to be a positive number")


Line is too long. [132/120]

houndci-bot · 2016-05-26T10:06:04Z

lib/shoryuken/manager.rb

+      end
+
+      unless @fetcher_pause_interval > 0
+        error_message = "Fetcher pause interval value #{@fetcher_pause_interval} is invalid, it needs to be a positive number"


Line is too long. [126/120]

bbaja42 · 2016-05-26T10:10:30Z

lib/shoryuken/manager.rb

+        raise(ArgumentError, "Concurrency value #{@count} is invalid, it needs to be a positive number")
+      end
+
+      unless @fetcher_pause_interval > 0


I think I experimented with this value being set to 0 without any problems.
https://github.com/celluloid/timers/blob/fcb443006ed1770d3ba2ebf3db0e0d2e570c6672/lib/timers/group.rb#L37
So is perhaps instead if @fetcher_pause_interval < 0 is better?

bbaja42 · 2016-05-26T10:11:34Z

lib/shoryuken/manager.rb

@@ -131,7 +140,7 @@ def dispatch
      if @ready.empty?
        logger.debug { 'Pausing fetcher, because all processors are busy' }


Could you add info in the message for how long is the fetcher being paused?

darraghotoole · 2016-05-26T10:12:04Z

@bbaja42 @mstewart Renamed from 'dispath_interval' to 'fetcher_pause_interval'. Now accepts 0.

phstc · 2016-05-31T13:19:17Z

lib/shoryuken/manager.rb

+
+      if @fetcher_pause_interval < 0
+        message = "Fetcher pause interval value #{@fetcher_pause_interval} is invalid, it cannot be negative"
+        raise(ArgumentError, message)


Unrelated to the PR: I'm wondering if we should move these ArgumentError errors to cli.rb, consequently failing in there, instead of later in the startup process. WDYT?

Makes sense, moving the relevant tests would also be the first tests against cli.rb, which seems to be untested at the moment.

Can you move that at least to the initialize method? That's where it's set.

phstc · 2016-05-31T13:19:52Z

Looks good @darraghotoole 🍻

@mariokostelac do you mind double checking it?

mariokostelac · 2016-06-01T16:28:03Z

lib/shoryuken/manager.rb

@@ -12,7 +12,17 @@ class Manager

    def initialize(condvar)
      @count = Shoryuken.options[:concurrency] || 25
-      raise(ArgumentError, "Concurrency value #{@count} is invalid, it needs to be a positive number") unless @count > 0
+      @fetcher_pause_interval = Shoryuken.options[:fetcher_pause_interval] || 1


we are using two different symbols:
:dispatch_interval and :fetcher_pause_interval.
Is that on purpose? I do not see anybody reading dispatch_interval

This was missed during refactor, good catch.

mariokostelac · 2016-06-01T16:30:13Z

It seems we can't set that value since we are using two different symbols.

darraghotoole · 2016-06-01T16:51:30Z

lib/shoryuken/cli.rb

@@ -119,6 +119,10 @@ def parse_cli_args(argv)
          opts[:concurrency] = Integer(arg)
        end

+        o.on '-i', '--fetcher-pause-interval DECIMAL', 'Interval to pause fetching when all workers are busy' do |arg|
+          opts[:fetcher_pause_interval] = Float(arg)


@mariokostelac Fixed ^^

mariokostelac · 2016-06-01T20:54:49Z

I've been thinking more about this one. You're solving a problem with optionally decreasing that time, but you're also introducing a problem of manager hitting the Q very often when there are no jobs. Making that value very close to 0 would mean constantly polling the Q.

I feel we can do very similar thing like I did in #215.
We are pretty sure that we can assign a new job to the worker in two situations:

when we just started/restarted a manager
when certain process is done with the work and moves from busy to ready collection, which could be when:
- job is done
- job crashed

We already call dispatch on start so have to do that same after updating @busy collection in processor_done and processor_died.

Using that approach you get total utilisation of your manager. You spend no time waiting for next dispatch cycle.

I am fine with adding this extra option on boot, but I'd like to make sure we are improving the code with every change.

Could we add those dispatch calls in the same PR @darraghotoole?

@phstc are you good with my approach?

phstc · 2016-06-02T11:52:48Z

@phstc are you good with my approach?

@mariokostelac I'm good with it.

I am fine with adding this extra option on boot, but I'd like to make sure we are improving the code with every change.

Loved :)

mariokostelac · 2016-06-04T08:07:31Z

Fix I've been talking about is merged in f4640d9.

eugeneius · 2016-06-06T15:12:02Z

lib/shoryuken/manager.rb

    def dispatch
      return if stopped?

      logger.debug { "Ready: #{@ready.size}, Busy: #{@busy.size}, Active Queues: #{unparse_queues(@queues)}" }

      if @ready.empty?
-        logger.debug { 'Pausing fetcher, because all processors are busy' }
+        logger.debug { 'Pausing fetcher for #{@fetcher_pause_interval} seconds, because all processors are busy' }


Single-quoted strings are not interpolated, so the log will contain the literal text:

Pausing fetcher for #{@fetcher_pause_interval} seconds, because all processors are busy

You probably want to use double quotes here.

eugeneius · 2016-06-06T15:20:28Z

This seems conceptually similar to the delay option: https://github.com/phstc/shoryuken/wiki/Shoryuken-options#delay

It's a bit inconsistent that delay can be set in the config file but not from the command line, and now this option will be settable from the command line but not in the config file.

mariokostelac · 2016-06-06T15:26:13Z

@eugeneius Yes, I do not see any useful difference right now. Since we've fixed the original issue on better way, I am inclined to avoid introducing extra complexity and close this PR.
@darraghotoole @mstewart @bbaja42 thanks for raising the issue and providing a solution for that. If you think that I am wrong with closing this one, please reopen.

houndci-bot reviewed May 25, 2016
View reviewed changes

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from 86ce4b0 to b1c3d3d Compare May 25, 2016 14:14

houndci-bot reviewed May 25, 2016
View reviewed changes

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from b1c3d3d to a6df078 Compare May 26, 2016 10:04

houndci-bot reviewed May 26, 2016
View reviewed changes

darraghotoole changed the title ~~Make dispatch interval configurable~~ Make fetcher pause interval configurable May 26, 2016

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from a6df078 to 5ad43ca Compare May 26, 2016 10:05

houndci-bot reviewed May 26, 2016
View reviewed changes

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from 5ad43ca to 73bfced Compare May 26, 2016 10:09

bbaja42 reviewed May 26, 2016
View reviewed changes

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from 73bfced to f66b73d Compare May 26, 2016 10:11

bbaja42 reviewed May 26, 2016
View reviewed changes

darraghotoole force-pushed the dot/configurable_dispatch_interval branch 3 times, most recently from 252b673 to a837a71 Compare May 26, 2016 10:18

bbaja42 mentioned this pull request May 26, 2016

Long polling #30

Closed

phstc reviewed May 31, 2016
View reviewed changes

mariokostelac reviewed Jun 1, 2016
View reviewed changes

Make dispatch pause interval configurable

f1661e8

darraghotoole force-pushed the dot/configurable_dispatch_interval branch from a837a71 to f1661e8 Compare June 1, 2016 16:43

darraghotoole reviewed Jun 1, 2016
View reviewed changes

eugeneius reviewed Jun 6, 2016
View reviewed changes

mariokostelac closed this Jun 6, 2016

darraghotoole deleted the dot/configurable_dispatch_interval branch June 7, 2016 08:54

mariokostelac mentioned this pull request Dec 23, 2016

WIP: native threads #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fetcher pause interval configurable #214

Make fetcher pause interval configurable #214

darraghotoole commented May 25, 2016 •

edited

Loading

houndci-bot May 25, 2016

houndci-bot May 25, 2016

houndci-bot May 26, 2016

houndci-bot May 26, 2016

bbaja42 May 26, 2016

bbaja42 May 26, 2016

darraghotoole commented May 26, 2016 •

edited

Loading

phstc May 31, 2016

darraghotoole Jun 1, 2016

mariokostelac Jun 4, 2016

phstc commented May 31, 2016

mariokostelac Jun 1, 2016 •

edited

Loading

darraghotoole Jun 1, 2016

mariokostelac commented Jun 1, 2016

darraghotoole Jun 1, 2016

mariokostelac commented Jun 1, 2016 •

edited

Loading

phstc commented Jun 2, 2016

mariokostelac commented Jun 4, 2016

eugeneius Jun 6, 2016

eugeneius commented Jun 6, 2016

mariokostelac commented Jun 6, 2016

		@@ -131,7 +140,7 @@ def dispatch
		if @ready.empty?
		logger.debug { 'Pausing fetcher, because all processors are busy' }

Make fetcher pause interval configurable #214

Make fetcher pause interval configurable #214

Conversation

darraghotoole commented May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darraghotoole commented May 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phstc commented May 31, 2016

mariokostelac Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariokostelac commented Jun 1, 2016

Choose a reason for hiding this comment

mariokostelac commented Jun 1, 2016 • edited Loading

phstc commented Jun 2, 2016

mariokostelac commented Jun 4, 2016

Choose a reason for hiding this comment

eugeneius commented Jun 6, 2016

mariokostelac commented Jun 6, 2016

darraghotoole commented May 25, 2016 •

edited

Loading

darraghotoole commented May 26, 2016 •

edited

Loading

mariokostelac Jun 1, 2016 •

edited

Loading

mariokostelac commented Jun 1, 2016 •

edited

Loading