implement goroutine_pool package and integrate #1766
Conversation
|
Warning Rate limit exceeded@Intizar-T has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 18 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe update introduces a worker pool pattern to the codebase, optimizing parallel job execution across various modules. A new Changes
Poem
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- node/pkg/fetcher/accumulator.go (2 hunks)
- node/pkg/raft/raft.go (4 hunks)
- node/pkg/utils/goroutine-pool/pool.go (1 hunks)
- node/pkg/utils/tests/pool_test.go (1 hunks)
Additional comments not posted (9)
node/pkg/utils/goroutine-pool/pool.go (5)
7-13: LGTM! Type definitions and constants are correct.The type definitions for
PoolJobandPool, along with the constantPOOL_WORKER_COUNT, are well-defined.
15-19: LGTM! Pool initialization is correct.The
NewPoolfunction correctly initializes a newPoolwith a job channel.
21-25: LGTM! Worker goroutines are started correctly.The
Runfunction correctly starts worker goroutines and handles context cancellation.
27-35: LGTM! Worker function is implemented correctly.The
workerfunction correctly processes jobs from the job channel and handles context cancellation.
38-40: LGTM! Job addition is implemented correctly.The
AddJobfunction correctly adds a job to the job channel.node/pkg/utils/tests/pool_test.go (1)
11-16: LGTM! Pool creation test is correct.The
TestNewPoolfunction correctly tests the creation of a new pool.node/pkg/fetcher/accumulator.go (2)
28-33: LGTM! Ticker and context handling are correct.The ticker and context handling in the
Runmethod are correctly implemented.
34-36: LGTM! Job addition to the pool is correct.The accumulator job is correctly added to the pool.
node/pkg/raft/raft.go (1)
Line range hint
323-341:
LGTM! Ticker and context handling are correct.The ticker and context handling in the
becomeLeadermethod are correctly implemented.
Intizar-T
force-pushed
the
feat/aggregator-goroutine-pooling
branch
from
3f86256
to
6e7f64c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
node/pkg/utils/pool/pool.go (2)
25-33: Consider adding logging for job processing and context cancellation.Adding logging would help in debugging and monitoring the worker's activity.
- select { + select { + case job := <-p.jobChannel: + log.Println("Processing job") + job() + case <-ctx.Done(): + log.Println("Worker exiting") + return
36-43: Consider adding logging for adding jobs and context cancellation.Adding logging would help in debugging and monitoring job addition.
- select { + select { + case p.jobChannel <- job: + log.Println("Job added to the pool") + return + case <-ctx.Done(): + log.Println("Add job operation cancelled") + return
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- node/pkg/fetcher/accumulator.go (2 hunks)
- node/pkg/raft/raft.go (4 hunks)
- node/pkg/utils/pool/pool.go (1 hunks)
- node/pkg/utils/tests/pool_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- node/pkg/fetcher/accumulator.go
- node/pkg/raft/raft.go
- node/pkg/utils/tests/pool_test.go
Additional comments not posted (2)
node/pkg/utils/pool/pool.go (2)
7-10: Struct definition looks good.The
Poolstruct is well-defined with a job channel and worker count.
19-23: Method looks good.The
Runmethod correctly spawns the worker goroutines.
|
can you also include Stop() func in pool package? and I think it should be integrated with other package's start and stop. to do so, I recommend you to include pool into accumulator and raft structure. rather than initializing pool from run, it'd be better aligned if the pools are initialized from each init funcs for both accumulator and raft |
|
in raft, pool stop should be included in raft.ResignLeader |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node/pkg/utils/tests/pool_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- node/pkg/utils/tests/pool_test.go
|
@nick-bisonai i intentionally didn't implement context cancel in the |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node/pkg/utils/tests/pool_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- node/pkg/utils/tests/pool_test.go
I understand, but in my perspective, I thought that using the temporarily declared variable inside run function seemed less structured, and less managing. if you want to keep the code please add test codes to check if there aren't wasted resources after leader resign or accumulator stop. There shouldn't be any running pools after those steps. since it's running in separate go routine, I think it should be signaled somehow for cleanup if it is not used anymore |
@nick-bisonai could you pls elaborate more on what you meant by And yes, there is a test that ensures Pool workers exit after context is canceled. I think the current implementation should be safe since |
I was talking about this pattern inside Run functions for resignLeader doesn't send ctx.cancel to the pool object you have said. but it should be canceled somehow. for accumulator it might work as you expected since it uses cancel which is defined from init func |
|
if you want to keep the pattern you should add something which cancels pool context from function but it still looks more organized for me to have internal pool property for both accumulator and raft, and externally specifying and for the test, I was meant for test from accumulator side and raft side. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- node/pkg/aggregator/aggregator.go (2 hunks)
- node/pkg/fetcher/accumulator.go (2 hunks)
- node/pkg/raft/raft.go (6 hunks)
- node/pkg/raft/types.go (1 hunks)
- node/pkg/reporter/reporter.go (2 hunks)
- node/pkg/utils/pool/pool.go (1 hunks)
- node/pkg/utils/tests/pool_test.go (1 hunks)
- node/script/test_raft/main.go (2 hunks)
Files skipped from review due to trivial changes (1)
- node/pkg/raft/types.go
Files skipped from review as they are similar to previous changes (4)
- node/pkg/fetcher/accumulator.go
- node/pkg/raft/raft.go
- node/pkg/utils/pool/pool.go
- node/pkg/utils/tests/pool_test.go
Additional comments not posted (6)
node/script/test_raft/main.go (2)
14-14: Define WORKER_COUNT constant.The constant
WORKER_COUNTis defined correctly and will be used to set the number of workers for the goroutine pool.
49-49: Update NewRaftNode call to include WORKER_COUNT.The function call is correctly updated to pass
WORKER_COUNT, initializing the worker pool with the specified number of workers.node/pkg/reporter/reporter.go (2)
20-20: Define WORKER_COUNT constant.The constant
WORKER_COUNTis defined correctly and will be used to set the number of workers for the goroutine pool.
51-51: Update NewRaftNode call to include WORKER_COUNT.The function call is correctly updated to pass
WORKER_COUNT, initializing the worker pool with the specified number of workers.node/pkg/aggregator/aggregator.go (2)
20-20: Define WORKER_COUNT constant.The constant
WORKER_COUNTis defined correctly and will be used to set the number of workers for the goroutine pool.
37-37: Update NewRaftNode call to include WORKER_COUNT.The function call is correctly updated to pass
WORKER_COUNT, initializing the worker pool with the specified number of workers.
|
@nick-bisonai Pool now has it's own context management. It gets cancelled from |
node/pkg/utils/pool/pool.go
Outdated
| } | ||
|
|
||
| func (p *Pool) AddJob(job func()) { | ||
| select { |
There was a problem hiding this comment.
if !isRunning {
return
}
There was a problem hiding this comment.
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 as they are similar to previous changes (1)
- node/pkg/utils/pool/pool.go
Intizar-T
force-pushed
the
feat/aggregator-goroutine-pooling
branch
from
e472b54
to
bbf2362
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- node/pkg/aggregator/aggregator.go (2 hunks)
- node/pkg/fetcher/accumulator.go (2 hunks)
- node/pkg/raft/raft.go (6 hunks)
- node/pkg/raft/types.go (1 hunks)
- node/pkg/reporter/reporter.go (2 hunks)
- node/pkg/utils/pool/pool.go (1 hunks)
- node/pkg/utils/tests/pool_test.go (1 hunks)
- node/script/test_raft/main.go (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- node/pkg/aggregator/aggregator.go
- node/pkg/fetcher/accumulator.go
- node/pkg/raft/raft.go
- node/pkg/raft/types.go
- node/pkg/reporter/reporter.go
- node/pkg/utils/pool/pool.go
- node/pkg/utils/tests/pool_test.go
- node/script/test_raft/main.go
Description
Accumulatorandraft'sbecomeLeaderfunctionFixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment