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

Merged

Conversation

Th3-M4jor
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor 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
@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
@Th3-M4jor Th3-M4jor force-pushed the add-ids-to-workers-yield-self-to-lifecycle-hooks branch from cdd9bf2 to 62ba7b0 Compare March 5, 2025 23:33
@rosa
Copy link
Member

rosa commented Mar 14, 2025

Nice! Thanks a lot and sorry for the delay!

@rosa rosa merged commit a9ee00a into rails:main Mar 14, 2025
34 checks passed
@Th3-M4jor Th3-M4jor deleted the add-ids-to-workers-yield-self-to-lifecycle-hooks branch March 14, 2025 13:53
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