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

Implement waiting max concurrency #34

Open
cep21 opened this issue Feb 5, 2018 · 8 comments
Open

Implement waiting max concurrency #34

cep21 opened this issue Feb 5, 2018 · 8 comments

Comments

@cep21
Copy link
Owner

cep21 commented Feb 5, 2018

Add a feature that uses https://github.com/golang/sync/blob/master/semaphore/semaphore.go to limit concurrency. In this implementation, rather than failing fast with an error, the function would wait according to the context if was at a limit of MaxConcurrentRequests

@ray1888
Copy link

ray1888 commented May 7, 2020

have this issue is still on to do status or it has been finished?

@cep21
Copy link
Owner Author

cep21 commented May 7, 2020

It is still todo. The more I thought about it, the more worried I became that it would be dangerous. Do you have a strong need for this: curious about your use case.

@ray1888
Copy link

ray1888 commented May 8, 2020

for me, it is not strong need for me, but i am curious on this issue. i want to contribute this with a PR, but maybe not that fast, this required me to finish in spare time

@cep21
Copy link
Owner Author

cep21 commented May 8, 2020

For this feature, I worry the circuit code is already complex and it may be best if this was implemented as a wrapper on top of the circuit. It may look like something like this

type WaitingConcurrency struct {
  sem *semaphore.Weighted
  wrapped circuit.Circuit
}

func (w *WaitingConcurrency) Execute(ctx context.Context, runFunc func(context.Context) error, fallbackFunc func(context.Context, error) error) error {
  w.sem.Acquire(ctx)
  defer w.sem.Release(1)
  return w.wrapped.Execute(ctx, runFunc, fallbackFUnc)
}

The exact code will probably be a bit different and maybe require some good error checking, but the idea is to move this code outside of this already complex package into a wrapper.

If you want to contribute something like this on your personal github, I would gladly accept a pull request to reference your implementation inside this repositories README file..

Let me know if you need more details.

@ray1888
Copy link

ray1888 commented May 9, 2020

so what i should do in this implementation? iam still not that much understand the demand of this issue, what i am understanding accroding to u

 In this implementation, rather than failing fast with an error, the function would wait according to the context if was at a limit of MaxConcurrentRequests

is that should i implement a wrapper and handling the concurrency without affecting the code of exist circuit?

@ray1888
Copy link

ray1888 commented May 9, 2020

so far i dive deep into this code, is just burtually return error when inside circuit.run() allowNewRun just return false

@cep21
Copy link
Owner Author

cep21 commented May 12, 2020

return error when inside circuit.run() allowNewRun

Rather than modify any code inside this repository, create a new struct that uses a circuit as a member.

@amanbolat
Copy link

I think there is no need to use semaphores for that. When we need max concurrency set it is better to fail fast and create back pressure, rather than have bunch of stalled requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants