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

Basic Implementation Making Queue Size Configurable by Workers #53

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gxuu
Copy link

@gxuu gxuu commented Jan 31, 2025

I've rolled a basic solution according to the requirements outlined in issue #51. I welcome any comments and advice to further improve the program.

@gxuu
Copy link
Author

gxuu commented Jan 31, 2025

More detail in the discussion of issue #51. This is not ready to merge and I plan to make improvement after receiving feedback.

As instructed, this commit does the following things.

- Use orignal name instead of creating new APIs.
  Note, this means breaking change to existing code. The original
  per_worker_queue_size is removed.

- Change related functions in entry_point/ directory.

- Modify test to cooperate with new function signatures.

- Rewrite the balance algorithm.
@gxuu gxuu reopened this Feb 7, 2025
@gxuu
Copy link
Author

gxuu commented Feb 7, 2025

UPD:

This commit refactors the worker queue handling and balance algorithm. The changes include:

  • Modified the original function signature instead of introducing new APIs.
  • Updated the entry_point/ implementation to use the new functions.
  • Removed references to per_worker_queue_size.
  • Allowed worker queue sizes to be initialized externally, both as command-line arguments and function parameters.
  • Revised the implementation for symphony workers.
  • Modified the balance algorithm.
  • Updated all test implementations to use the new functions.

Note on design:

  • per_worker_queue_size is probably worth keeping. It can be used as default when queue sizes are not provided. My opinion is that the name is confusing, and therefore I removed it for now.
  • Symphony CLUSTER seems to have only a single worker instead of a cluster. I am not sure whether this is intended or a work in progress. For now, the worker in symphony cluster only have one queue size.
  • worker queue sizes doesn't have a default value for now, but it might be worth adding one. Otherwise, the code looks nasty.

I would like to receive one more round of feedback before cleaning up the mess. That said, I think all the changes that needs to be done are done.

gxu

@gxuu gxuu requested a review from 1597463007 February 7, 2025 19:31
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.

2 participants