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

Whitespace in queues configuration can cause issues. #1351

Closed
TAGraves opened this issue May 16, 2024 · 2 comments · Fixed by #1352
Closed

Whitespace in queues configuration can cause issues. #1351

TAGraves opened this issue May 16, 2024 · 2 comments · Fixed by #1352

Comments

@TAGraves
Copy link
Contributor

We had the following queues config for a worker:

"queues": "+workload_5s: 3; +workload_15s,workload_5s:3; +workload_1m,workload_15s,workload_5s:2; +workload_2m,workload_1m,workload_15s,workload_5s:2; +workload_5m,workload_2m,workload_1m,workload_15s,workload_5s:2",

We observed that with this configuration, workload_5m jobs were never being worked. I removed all the spaces from the above configuration:

"queues": "+workload_5s:3;+workload_15s,workload_5s:3;+workload_1m,workload_15s,workload_5s:2;+workload_2m,workload_1m,workload_15s,workload_5s:2;+workload_5m,workload_2m,workload_1m,workload_15s,workload_5s:2",

Then the workload_5m jobs started getting picked up. So it seems like the space between the ; and the +workload_5m, was causing an issue. I don't see it documented anywhere that whitespace is relevant, and indeed the readme even has several examples of whitespace being used in the queue configuration, so I suspect this is a bug.

@bensheldon
Copy link
Owner

GoodJob should be whitespace tolerant. I'm probably missing some strip's 🤔 Here's where the string gets broken down into a configuration hash:

  • Where the configuration constant is broken down:

    queue_string, max_threads = queue_string_and_max_threads.split(':')
    max_threads = (max_threads || configuration.max_threads).to_i
    job_performer = GoodJob::JobPerformer.new(queue_string)

  • Where the comma-separated list of queues gets broken down:

    def self.queue_parser(string)
    string = string.presence || '*'
    case string.first
    when '-'
    exclude_queues = true
    string = string[1..]
    when '+'
    ordered_queues = true
    string = string[1..]
    end
    queues = string.split(',').map(&:strip)
    if queues.include?('*')
    { all: true }
    elsif exclude_queues
    { exclude: queues }
    elsif ordered_queues
    {
    include: queues,
    ordered_queues: true,
    }
    else
    { include: queues }
    end
    end

Also, I love how you've set up your queues 😍

Let me make a test case, but I think it needs more striping

@bensheldon
Copy link
Owner

Also, I love how you've set up your queues 😍

Unrelated: my one piece of feedback would be not to use the +-queue ordering. That has performance downsides and you likely want to clear faster-SLO jobs first rather than the slower-SLO ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants