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

Enable MultiProcessor in processor.go #217

Merged
merged 11 commits into from
Apr 22, 2024
Merged

Conversation

karenychen
Copy link
Contributor

@karenychen karenychen commented Apr 16, 2024

User can add more than one receiver to listen to with go-shuttle's Processor.

Use NewMultiProcessor() to initialize a list of receivers in a Processor

Note: max concurrency count is shared across all receivers.

MultiProcessor is not yet compatible with LockRenewer.

@coveralls
Copy link
Collaborator

coveralls commented Apr 16, 2024

Pull Request Test Coverage Report for Build 8776717166

Details

  • 81 of 94 (86.17%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 85.417%

Changes Missing Coverage Covered Lines Changed/Added Lines %
v2/processor.go 76 79 96.2%
v2/metrics/processor/types.go 5 15 33.33%
Files with Coverage Reduction New Missed Lines %
v2/metrics/processor/types.go 2 80.73%
v2/lockrenewer.go 3 95.24%
Totals Coverage Status
Change from base Build 8699030379: -0.2%
Covered Lines: 738
Relevant Lines: 864

💛 - Coveralls

v2/processor.go Outdated Show resolved Hide resolved
@karenychen karenychen marked this pull request as ready for review April 16, 2024 22:25
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
v2/processor.go Outdated Show resolved Hide resolved
wg := sync.WaitGroup{}
errChan := make(chan error, len(p.receivers))
for name := range p.receivers {
wg.Add(1)
Copy link
Member

@serbrech serbrech Apr 19, 2024

Choose a reason for hiding this comment

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

Here, if startOne panics, or if the cahnnel is closed for example, and we try to push err on it, we will not call wg.Done() because we don't recover from the panic, I believe.
So I think that the wg.Wait() will keep us stuck forever.

could you try to add a test for that by generating a panic somehow inside startOne ?

Copy link
Member

Choose a reason for hiding this comment

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

in terms of fixing it, I've been itching to use conc library in go-shuttle, but unsure whether that's wise as a library.
we should handle the panic I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added panic handling + UTs

Copy link
Member

@serbrech serbrech left a comment

Choose a reason for hiding this comment

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

Ok let's get this in. we might want to adjust some things once we write integration code around it.

@karenychen karenychen merged commit 1fd7ed8 into main Apr 22, 2024
5 checks passed
@karenychen karenychen deleted the karenchen/multiprocessor branch April 23, 2024 00:05
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.

None yet

4 participants