Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

functions: common concurrency stream for sync and async #314

Merged
merged 23 commits into from
Nov 18, 2016

Conversation

ucirello
Copy link
Contributor

@ucirello ucirello commented Nov 16, 2016

This is a proposal that addresses concerns some aspects of #165 and #69.

1 - We have now a supervised goroutine that spawns workers for the incoming load. They serve any kind of request, sync or async (#165)

2 - Because runners start are separate from request life-cycle, we would be a third way closer to have hot reusable containers (#69)

Edit:
Implementation Details

This PR introduces new goroutine responsible solely for executing tasks. It interfaces with the HTTP server and async modules through a channel, whose contents holds both the task information, and a channel to send the answer backer to the caller. https://github.com/iron-io/functions/blob/bounded-concurrency/api/server/runner.go#L183-L188 and https://github.com/iron-io/functions/blob/bounded-concurrency/api/runner/async_runner.go#L142-L145

In order to prevent resource starvation between competing sync and async tasks, we reserve a maxium of 50% of the memory for async tasks. This number is arbitrary and might be subject to changes later. https://github.com/iron-io/functions/blob/bounded-concurrency/api/runner/runner.go#L118-L123

In practice, when the memory allocated for async workers is maxed out, it keeps looping until memory is available: https://github.com/iron-io/functions/blob/bounded-concurrency/api/runner/async_runner.go#L117-L121

Also relevant is that Server now fathers handleRequest https://github.com/iron-io/functions/blob/bounded-concurrency/api/server/runner.go#L37 - with which it injects the tasks channel into the HTTP server.

@ucirello ucirello changed the title [WIP] functions: add bounded concurrency [Proposal] functions: add bounded concurrency Nov 16, 2016
@treeder
Copy link
Contributor

treeder commented Nov 17, 2016

Couple questions @ccirello:

  • The work done in Wait for capacity if no RAM left on server and log wait time as metric to logs #10 should have made it bound to the max number of runners, with a small buffer (100 requests), then the rest get rejected. Unless that didn't get implemented like the issue description? cc @pedronasser
  • What's the point of resizing the number of runners? There's always some fixed limit to the memory available right at the start. We can't go above it and there's no loss for keeping them running when below.

@ucirello
Copy link
Contributor Author

@treeder now that I have the information that the runner regulates itself, there's no need for max runners. I am going to remove this extra mechanism.

@ucirello ucirello changed the title [Proposal] functions: add bounded concurrency [Proposal] functions: common concurrency stream for sync and async Nov 17, 2016
@ucirello ucirello added this to the Alpha 2 milestone Nov 17, 2016
@ucirello ucirello force-pushed the bounded-concurrency branch from ee7d4cf to 993e1f9 Compare November 17, 2016 01:14
@ucirello ucirello changed the title [Proposal] functions: common concurrency stream for sync and async [WIP] functions: common concurrency stream for sync and async Nov 17, 2016
@ucirello ucirello changed the title [WIP] functions: common concurrency stream for sync and async functions: common concurrency stream for sync and async Nov 17, 2016
Copy link
Contributor

@pedronasser pedronasser left a comment

Choose a reason for hiding this comment

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

LGTM

@seiflotfy seiflotfy merged commit 9d06b6e into master Nov 18, 2016
@ucirello ucirello deleted the bounded-concurrency branch November 18, 2016 17:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants