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

Fixes #31964 - Assign equal weight to sidekiq queues #927

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Mar 25, 2021

The previous approach worker, but it made sidekiq default worker process all items from the default queue before moving to remote execution queue. This change adds equal weight to the queues, making sidekiq pick jobs from both.

To achieve this a function is introduced to convert data to YAML. Foreman typically uses symbolized keys in config files. This is hard to achieve with pure Puppet so a Ruby function is used.

Replaces #919

# # output yaml to a file
# file { '/tmp/my.yaml':
# ensure => file,
# content => to_yaml($myhash),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# content => to_yaml($myhash),
# content => foreman::to_symbolized_yaml($myhash),

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you caught my copy-paste

# @example Use options control the output format
# file { '/tmp/my.yaml':
# ensure => file,
# content => to_yaml($myhash, {indentation: 4})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# content => to_yaml($myhash, {indentation: 4})
# content => foreman::to_symbolized_yaml($myhash, {indentation: 4})

@ehelms
Copy link
Member

ehelms commented Mar 31, 2021

Will this handle:

  • Nested hashes
  • Hashes inside arrays

@ekohl
Copy link
Member Author

ekohl commented Apr 1, 2021

  • Nested hashes

  • Hashes inside arrays

No. I chose not to implement that. Partly because I was lazy, partly because I don't know if there is any demand for it. It may even break in some cases.

@ekohl ekohl force-pushed the 31964-equal-weights branch from 5ab2719 to 1f85ee2 Compare April 1, 2021 17:35
@ekohl
Copy link
Member Author

ekohl commented Apr 1, 2021

Rebased to resolve conflicts.

@ekohl ekohl force-pushed the 31964-equal-weights branch from 1f85ee2 to 6f93ad9 Compare April 1, 2021 18:02
ekohl added 2 commits April 6, 2021 17:10
This function symbolizes keys in YAML which is typically what Foreman
config files use. This is hard to achieve with pure Puppet so a Ruby
function is used.
The previous approach worker, but it made sidekiq default worker process all
items from the default queue before moving to remote execution queue. This
change adds equal weight to the queues, making sidekiq pick jobs from both.
@ekohl ekohl force-pushed the 31964-equal-weights branch from 6f93ad9 to 4567278 Compare April 6, 2021 15:10
<<~CONTENT
---
:concurrency: 1
:queues:
Copy link
Member

Choose a reason for hiding this comment

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

queues can either be a list or a list of lists? Thats right ugly but I realize not our "concern" per this feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is indeed my impression based on @adamruzicka's patch. I think we're just exposing what Dynflow accepts.

@adamruzicka can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a sidekiq-specific thing[1], Dynflow has no role in this. It can be either a list or a list of two element lists, where first element is name of the queue and the other is the priority.

[1] - https://github.com/mperham/sidekiq/wiki/Advanced-Options#queues

@ekohl ekohl merged commit 616f172 into theforeman:master Apr 7, 2021
@ekohl ekohl deleted the 31964-equal-weights branch April 7, 2021 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants