-
Notifications
You must be signed in to change notification settings - Fork 19
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
(DAL) Pooling broadcast #2246
(DAL) Pooling broadcast #2246
Conversation
WalkthroughWalkthroughThe changes introduce a worker pool to the Changes
Sequence Diagram(s)sequenceDiagram
participant H as Hub
participant P as Pool
participant C as Collector
H->>H: NewHub(configs)
H->>P: Initialize pool
H->>C: Start(ctx)
C->>P: Run(ctx)
H->>P: AddJob(task)
P->>P: runWorker(ctx)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- node/pkg/dal/hub/hub.go (4 hunks)
- node/pkg/utils/pool/pool.go (1 hunks)
Files skipped from review due to trivial changes (1)
- node/pkg/utils/pool/pool.go
Additional comments not posted (5)
node/pkg/dal/hub/hub.go (5)
33-33
: Approved structural change toHub
struct.The addition of the
pool
field is crucial for managing worker goroutines efficiently.The structural change is approved as it aligns with the PR objectives.
38-38
: Verify the appropriateness ofWORKER_COUNT
.The constant
WORKER_COUNT
is set to 20. It's important to ensure this number is optimized based on the expected load and system capabilities.Consider providing benchmarks or rationale for choosing this specific count.
59-59
: Approved changes inNewHub
constructor.Initializing the
pool
field within the constructor ensures that theHub
is ready to manage tasks using the worker pool.The change is approved as it is crucial for the functionality of the pooling mechanism.
64-64
: Approved changes inStart
method.The inclusion of
h.pool.Run(ctx)
is essential for activating the worker pool, allowing theHub
to handle tasks concurrently.The change is approved as it directly supports the PR objectives of enhancing concurrency management.
165-170
: Approved changes incastSubmissionData
method.Utilizing the worker pool in
castSubmissionData
simplifies concurrency management and is likely to improve performance. However, consider adding error handling for job submission failures.The changes are approved, but it's recommended to enhance robustness by handling potential job submission failures.
node/pkg/utils/pool/pool.go
Outdated
} | ||
} | ||
|
||
func (p *Pool) worker(ctx context.Context) { | ||
func (p *Pool) work(ctx context.Context) { |
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.
worker
in this context refers to goroutines that run the actual task/job/work. And seems like "In the context of a worker pool, the worker term is a widely accepted convention".
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.
I thought function name should be verb rather than noun, since it does some action rather than just a property or a value. how do you think? if it is worker
, at first sight I thought it should be some sort of struct or value
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.
well it seems trivial and minor so I'm not insisting, either is good for me. I'll rollback if you mind
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.
that's a valid point. But i found work
a bit weird as well lol. So maybe runWorker
? 😄
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.
lgtm! left a suggestion 🙏
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node/pkg/utils/pool/pool.go (1 hunks)
Files skipped from review due to trivial changes (1)
- node/pkg/utils/pool/pool.go
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node/pkg/dal/hub/hub.go (4 hunks)
Additional comments not posted (5)
node/pkg/dal/hub/hub.go (5)
33-33
: Addition ofpool
field inHub
struct approved.The new
pool
field is essential for the efficient management of goroutines, aligning well with the PR's objectives.The addition of the
pool
field is approved.
38-38
: Introduction ofWORKER_COUNT
constant approved.Setting this constant provides a clear reference point for tuning the number of workers in the pool, which is beneficial for scalability and performance adjustments.
The introduction of the
WORKER_COUNT
constant is approved.
59-59
: Initialization ofpool
inNewHub
function approved.Properly initializing the
pool
withpool.NewPool(WORKER_COUNT)
ensures that eachHub
instance can handle concurrent tasks efficiently.The changes in the
NewHub
function are approved.
64-64
: Starting the worker pool inStart
function approved.Calling
h.pool.Run(ctx)
at the beginning of theStart
method ensures that the worker pool is operational and ready to handle tasks concurrently.The changes in the
Start
function are approved.
165-170
: RefactoringcastSubmissionData
to use worker pool approved.Replacing manual synchronization with
h.pool.AddJob
simplifies the concurrency management and leverages the worker pool efficiently.The changes in the
castSubmissionData
function are approved.
This reverts commit 12378ae.
Description
Goroutine pooling on broadcasting data
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment