-
Notifications
You must be signed in to change notification settings - Fork 648
[Poc] Use Background pool to get JobInfo from Ray Dashboard #4043
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
base: master
Are you sure you want to change the base?
Changes from 8 commits
4069033
45635be
72142c0
fe90e70
727d2c2
d9a1a88
0f1f766
da0e45e
afcb9e1
82b3acf
69be5fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,58 @@ | ||||||
| package dashboardclient | ||||||
|
|
||||||
| import ( | ||||||
| "sync" | ||||||
|
|
||||||
| cmap "github.com/orcaman/concurrent-map/v2" | ||||||
| ) | ||||||
|
|
||||||
| type WorkerPool struct { | ||||||
| channelContent cmap.ConcurrentMap[string, struct{}] | ||||||
|
||||||
| taskQueue chan func() | ||||||
| stop chan struct{} | ||||||
| wg sync.WaitGroup | ||||||
| workers int | ||||||
| } | ||||||
|
|
||||||
| func NewWorkerPool(taskQueue chan func()) *WorkerPool { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing a task queue channel is weird. Specifying a worker count is more understandable. You can also make a buffered channel based on the worker count internally. |
||||||
| wp := &WorkerPool{ | ||||||
| taskQueue: taskQueue, | ||||||
| workers: 10, | ||||||
| stop: make(chan struct{}), | ||||||
| channelContent: cmap.New[struct{}](), | ||||||
| } | ||||||
|
|
||||||
| // Start workers immediately | ||||||
| wp.Start() | ||||||
| return wp | ||||||
| } | ||||||
|
|
||||||
| // Start launches worker goroutines to consume from queue | ||||||
| func (wp *WorkerPool) Start() { | ||||||
|
||||||
| for i := 0; i < wp.workers; i++ { | ||||||
| wp.wg.Add(1) | ||||||
| go wp.worker() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // worker consumes and executes tasks from the queue | ||||||
| func (wp *WorkerPool) worker() { | ||||||
| defer wp.wg.Done() | ||||||
|
|
||||||
| for { | ||||||
| select { | ||||||
| case <-wp.stop: | ||||||
| return | ||||||
| case task := <-wp.taskQueue: | ||||||
| if task != nil { | ||||||
| task() // Execute the job | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Stop shuts down all workers | ||||||
| func (wp *WorkerPool) Stop() { | ||||||
| close(wp.stop) | ||||||
| wp.wg.Wait() | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hide the jobInfoMap and workerPool implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to keep it if controller not directly call InitClient?
Currently controller will create a new DashboardClient for every reconciliation if we need to put the creation of workerPool , cmap .. in InitClient we will recreate them every reconciliation which is not what we want.
Current
GetRayDashboardClientFuncacts kind of like a factory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't they be created once in the GetRayDashboardClientFunc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like what you did with workerPool.