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

Have all processes yield self to lifecycle hooks #516

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Th3-M4jor
Copy link

@Th3-M4jor Th3-M4jor commented Feb 15, 2025

Makes it so that Workers will yield self to life cycle hooks and makes some adjustments related to what's public/private in them.

I did not do this for the dispatcher or supervisor as I felt that did not make sense.

closes #513

@Th3-M4jor Th3-M4jor force-pushed the add-ids-to-workers-yield-self-to-lifecycle-hooks branch from c2a0780 to 2a25d7f Compare February 15, 2025 18:05
@Th3-M4jor Th3-M4jor marked this pull request as ready for review February 15, 2025 18:09
@rosa
Copy link
Member

rosa commented Feb 18, 2025

Hey @Th3-M4jor, thanks for this!

However, I still don't understand the need for an incremental index for each worker 🤔 Could you elaborate more why this is necessary? These indexes would be duplicated across different supervisors (in the case you're running jobs in multiple machines).

All processes now have a unique generated name, which is used to locate them, and they carry other identifying information such as the hostname where they're running and their PID. Wouldn't this be more than enough to identify them?

@Th3-M4jor
Copy link
Author

Th3-M4jor commented Feb 18, 2025

For my particular case having an incremented index for each worker fits better for metrics reporting. It more closely mirrors how Puma identifies each of its forked workers, and I already inject the k8s pod name when reporting metrics to differentiate there.

Though I do see the point that it might not work for everyone's needs, what if I renamed worker_id to worker_index and added a worker_name which returned the unique generated name?

@rosa
Copy link
Member

rosa commented Feb 18, 2025

I think you don't need a worker_name 🤔 You can simply access it via #name, no?

About worker_index, if this is just for your particular case, I'd rather not add that, it should be something that's useful in general, not just for one particular way of reporting metrics. I think having the worker yielded in the lifecycle hook makes sense, but I don't see the need for having other attributes besides what is already there.

@Th3-M4jor Th3-M4jor force-pushed the add-ids-to-workers-yield-self-to-lifecycle-hooks branch from 0b5e0e7 to b89c29c Compare February 18, 2025 22:29
@Th3-M4jor Th3-M4jor changed the title Adds ids to workers and have them yield self to lifecycle hooks Have workers yield self to lifecycle hooks Feb 18, 2025
@Th3-M4jor
Copy link
Author

Fair enough, I removed all references to worker_id and squashed it down to a single commit. Accessing just the queues should work well enough for my case.

@rosa
Copy link
Member

rosa commented Feb 19, 2025

Cool! I think, for simplicity, we could have all processes do this, not just workers.

@Th3-M4jor Th3-M4jor force-pushed the add-ids-to-workers-yield-self-to-lifecycle-hooks branch from b89c29c to 1df5a2d Compare February 19, 2025 23:27
@Th3-M4jor
Copy link
Author

Done, and similarly made a few additional things on each process class private if they were only accessed via tests or from within the class itself.

@Th3-M4jor Th3-M4jor changed the title Have workers yield self to lifecycle hooks Have all processes yield self to lifecycle hooks Feb 19, 2025
SolidQueue.on_worker_stop do |worker|
Rails.logger.info "Worker #{worker.name} stopped with queues: #{worker.queues.join(',')}"
end
Copy link
Member

Choose a reason for hiding this comment

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

I know this is just because these are examples, but Solid Queue's log subscriber already logs all this when processes start and stop 🤔 I'm just not sure if it's ok to add examples that would be discouraged because you'd end up with duplicated/redundant logs.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to instead set a couple values in a somewhat contrived MyMetricsReporter class, which hopefully does a better job of showing what someone might want to use it for.

@Th3-M4jor Th3-M4jor force-pushed the add-ids-to-workers-yield-self-to-lifecycle-hooks branch from 1df5a2d to cdd9bf2 Compare February 25, 2025 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: include queues and worker index as arguments to on_worker_start hook
2 participants