Skip to content

Conversation

Ethereal-O
Copy link
Contributor

@Ethereal-O Ethereal-O commented May 30, 2025

Purpose of the PR

We would like to implement that when a new Worker is registered in the Master, it can be directly assigned to the specified WorkerGroup through the WorkerGroup name specified in the Worker configuration file.

Main Changes

  • Add worker_group configuration item to the default worker configuration file.
  • Worker loads the WorkerGroup configuration item from the configuration at startup.
  • Pass configuration items to the Master when SayHello.
  • The Master uses the passed WorkerGroup configuration item in the CreateWorker function to assign a Group to the Worker.
  • Check configuration items in the CreateWorker function. And remind the user when the data loaded from the object storage is different.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows.
    • Need to verify if the scheduler assigns tasks correctly.

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. feature New feature labels May 30, 2025
@imbajin imbajin requested a review from Copilot June 3, 2025 08:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables assigning a Worker to a specified WorkerGroup at registration time by propagating a new worker_group setting from the worker’s config through the SayHello RPC into the master’s CreateWorker logic.

  • Added a worker_group entry to the default worker config.
  • Loaded and passed worker_group from worker service into the proto request.
  • Extended master’s CreateWorker to accept, assign, and warn on group mismatches.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
vermeer/config/worker.ini Added new worker_group default setting.
vermeer/apps/worker/service.go Loaded worker_group via config and included in HelloMasterReq.
vermeer/apps/protos/master.proto Added WorkerGroup field to HelloMasterReq.
vermeer/apps/master/workers/worker_manager.go Updated CreateWorker signature to accept and set workerGroup.
vermeer/apps/master/bl/grpc_handlers.go Passed WorkerGroup into CreateWorker and enhanced log output.
Comments suppressed due to low confidence (3)

vermeer/apps/worker/service.go:72

  • Using a magic default value "$" is unclear; consider defining a constant (e.g., DefaultWorkerGroup) or using the actual default group name ("default") to avoid confusion.
workerGroup := common.GetConfigDefault("worker_group", "${}").(string)

vermeer/apps/master/workers/worker_manager.go:120

  • The literal "$" is used as a fallback; consider replacing it with a named constant (e.g., DefaultWorkerGroup) for clarity and to centralize the default value.
if workerGroup == "" { workerGroup = "$" }

vermeer/apps/master/workers/worker_manager.go:107

  • There are no tests covering the new workerGroup parameter or the mismatch warning logic; consider adding unit tests to verify correct group assignment and warning behavior.
func (wm *workerManager) CreateWorker(workerPeer string, ipAddr string, version string, workerGroup string) (*WorkerClient, error) {

@hankwenyx
Copy link

Well done, I have no other questions.
@imbajin This PR can be approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assign WorkerGroup via worker configuration
2 participants