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

Executor: force Function Executor RunTask concurrency=1 #1138

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

eabatalov
Copy link
Contributor

@eabatalov eabatalov commented Dec 24, 2024

Previously before in-Process Function Executor was introduced max Function concurrency in Executor was 1. We bring the previous behavior here but enforce the concurrency of 1 per Function Executor (not for the whole Executor). It's better this way cause it's hard for customers to reason about implications of running multiple concurrent tasks per Function Executor.

Also added tests that verify task concurrency for the same and different functions.

We'll revise this concurrency policy in the future if we allow customers to configure their functions' concurrency explicitly via function attributes.

I also did a big refactoring which results in task policy implementation simplification. This is how the current task policies are implemented now:

await state.wait_running_tasks_less(1)

if state.function_id_with_version != _function_id_with_version(task):
    await state.destroy_function_executor()
    state.function_id_with_version = _function_id_with_version(task)

There also many other refactorings in this PR that help to implement the upcoming features in Executor and simplify the code base.

Testing:

make check
make test

Contribution Checklist

  • If the python-sdk was changed, please run make fmt in python-sdk/.
  • [n/a] If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

Previously before in-Process Function Executor was introduced
max task concurrency in Executor was 1. We bring the previous
behavior here but enforce the concurrency of 1 per Function Executor
(not for the whole Executor). It's better this way cause it's
hard for customers to reason about implications of running multiple
concurrent tasks per Function Executor.

Also added tests that verify task concurrency for the same and
different functions.

We'll revise this concurrency policy in the future if we allow
customers to configure their functions' concurrency explicitly
via function attributes.

I also did a big refactoring which results in task policy implementation
simplification. This is how the current task policies are implemented now:

```
await state.wait_running_tasks_less(1)

if state.function_id_with_version != _function_id_with_version(task):
    await state.destroy_function_executor()
    state.function_id_with_version = _function_id_with_version(task)
```

There also many other refactorings in this PR that help to implement
the upcoming features in Executor and simplify the code base.

Testing:

make check
make test
@eabatalov eabatalov force-pushed the eugene-executor-task-queue-single-task-concurrency branch from 9eace93 to 0ce8d42 Compare December 24, 2024 17:38
@eabatalov eabatalov marked this pull request as ready for review December 24, 2024 17:39
@eabatalov eabatalov merged commit 2f7e3d6 into main Dec 24, 2024
8 checks passed
@eabatalov eabatalov deleted the eugene-executor-task-queue-single-task-concurrency branch December 24, 2024 18:10
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.

1 participant