Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Rename Sidekiq::Worker to Sidekiq::Job #4955

Closed
mperham opened this issue Aug 6, 2021 · 12 comments
Closed

Rename Sidekiq::Worker to Sidekiq::Job #4955

mperham opened this issue Aug 6, 2021 · 12 comments

Comments

@mperham
Copy link
Collaborator

mperham commented Aug 6, 2021

The term worker within the Sidekiq ecosystem is a net negative. Replace instances in the wiki and codebase with more precise terminology, e.g.

Sidekiq::Worker -> Sidekiq::Job
worker -> process, thread, job depending on context

@seuros
Copy link
Collaborator

seuros commented Aug 6, 2021

This is going to be a major breaking change. Most addons of Sidekiq will need to be updated to work with the next version. Can you explain what the net negative in the term 'worker' ?

@mperham
Copy link
Collaborator Author

mperham commented Aug 6, 2021

I know, the plan is to have a very long backward compatibility period. Ideally we make migration as simple as search/replace "Sidekiq::Worker" -> "Sidekiq::Job". My initial thought is:

  • For 6.3.0, S::Worker will be an alias for S::Job
  • For 6.4.0 it will print a deprecation warning
  • For 7.0.0 it will break.

Restoring compatibility is as simple as Sidekiq::Worker = Sidekiq::Job in your initializer. It's quite possible 7.0 is still a year or more away since there's no major unreleased changes on the main branch.

"worker" is ambiguous, people use it to refer to processes ("I started a new worker"), types of jobs, executing threads ("I've got 10 workers"), jobs ("I enqueued 10 workers"), etc. It's a mess.

Does that make more sense?

@mperham
Copy link
Collaborator Author

mperham commented Aug 6, 2021

The only major issue I've seen so far is that ActiveJob owns the app/jobs directory whereas Sidekiq owns app/workers. I don't have a solution for that but I'm not sure it's a major issue: is it strange to see app/workers/hard_job.rb?

@seuros
Copy link
Collaborator

seuros commented Aug 6, 2021

Hmm, I think this change will cause a major refactoring effort , a lot of gems reference to that module.
For example, it will render ActiveJob incompatible with Sidekiq 7 unless we monkey patch it .

For folder naming, if i had to not use app/workers and since we agreed that app/jobs if for ActiveJob wrappers, i will use app/async_tasks or app/sidekiq_tasks.

@mperham
Copy link
Collaborator Author

mperham commented Aug 7, 2021

I appreciate this feedback, you always have good advice.

I do want to create and document Sidekiq::Job. You are right that we can't realistically drop Sidekiq::Worker anytime soon, possibly forever. I will make the changes to use Sidekiq::Job everywhere but I will keep Sidekiq::Worker as a supported public API for the foreseeable future.

@mperham
Copy link
Collaborator Author

mperham commented Aug 19, 2021

Plan is to release 6.2.3 tomorrow without Sidekiq::Job. 6.3.0 will contain Sidekiq::Job, which will simply be an alias for Sidekiq::Worker.

@seuros
Copy link
Collaborator

seuros commented Aug 19, 2021

The alias is not a breaking change, so we could start testing it from 6.2.3 unofficially. Officially it will be 6.3.0. wdyt ?

@mperham
Copy link
Collaborator Author

mperham commented Aug 19, 2021

The issue is that the Sidekiq API currently has a helper class called Sidekiq::Job so I had to refactor the API a bit. That refactor is a breaking change for any scripts that referenced that class. It's not a major or documented API but it's still breaking so I thought it better to introduce it in a minor version bump, not a patch.

@mperham
Copy link
Collaborator Author

mperham commented Aug 19, 2021

Maybe it's probably better to introduce the breaking change now so people can fix their scripts and then introduce the new Sidekiq::Job alias in a future release?

@mperham
Copy link
Collaborator Author

mperham commented Aug 20, 2021

Latest plan:

  • Refactor API to remove Sidekiq::Job class name [6.2.x]
  • Introduce Sidekiq::Job as opt-in alias (need to require it) [6.2.x]
  • Make Sidekiq::Job a built-in alias [6.3]
  • Refactor all "worker" verbage in codebase and wiki to use more specific term [6.3+]

@seuros
Copy link
Collaborator

seuros commented Aug 21, 2021

@mperham sorry to comment here , but can you enable Discussions in this repo https://github.com/mperham/sidekiq/settings.

We could use it instead of have issues opened.

@mperham
Copy link
Collaborator Author

mperham commented Aug 21, 2021

Sure, we can try it out.

@sidekiq sidekiq locked and limited conversation to collaborators Aug 21, 2021
@seuros seuros closed this as completed Aug 21, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants