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

[Batcher] Add ability to send batched http requests to runtime #3244

Merged
merged 32 commits into from
Jun 10, 2024

Conversation

rokatyy
Copy link
Contributor

@rokatyy rokatyy commented May 7, 2024

Jira - https://iguazio.atlassian.net/browse/NUC-156 part of https://iguazio.atlassian.net/browse/NUC-146

This PR introduces a new events processing logic. Specifically, it allows the processing of batched events instead of the usual single event processing. For now, we only support this feature for python runtime and http trigger kind.

Batching can be enabled by adding batch configuration to function config.
Example:

batch:
        mode: enable
        batchSize: 1000
        timeout: 1s

@TomerShor TomerShor changed the title [WIP] Add ability to send patched http requests to runtime [WIP] Add ability to send batched http requests to runtime May 9, 2024
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

Some initial comments.
Will batching be supported in all triggers right OOB?

Since we only support it on python runtime for now, I would add a warning log that it is not supported if a different runtime is set (and maybe for triggers that don't support it too (e.g. shell))

pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/processor/runtime/rpc/abstract.go Outdated Show resolved Hide resolved
pkg/processor/runtime/runtime.go Outdated Show resolved Hide resolved
pkg/processor/trigger/batcher.go Show resolved Hide resolved
pkg/processor/trigger/batcher.go Outdated Show resolved Hide resolved
pkg/processor/trigger/batcher.go Outdated Show resolved Hide resolved
pkg/processor/trigger/http/trigger.go Outdated Show resolved Hide resolved
@rokatyy rokatyy changed the title [WIP] Add ability to send batched http requests to runtime [Batcher] Add ability to send batched http requests to runtime May 22, 2024
@rokatyy rokatyy marked this pull request as ready for review May 22, 2024 12:54
Copy link
Contributor

@quaark quaark left a comment

Choose a reason for hiding this comment

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

Looks really good!
A couple of minor suggestions

pkg/processor/trigger/http/trigger.go Outdated Show resolved Hide resolved
pkg/processor/trigger/trigger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

Awesome stuff!
Added a few comments

pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Show resolved Hide resolved
pkg/processor/runtime/rpc/abstract.go Outdated Show resolved Hide resolved
pkg/processor/runtime/rpc/abstract.go Show resolved Hide resolved
pkg/processor/trigger/trigger.go Outdated Show resolved Hide resolved
pkg/processor/trigger/trigger.go Outdated Show resolved Hide resolved
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

A bit more to go :)

pkg/functionconfig/types.go Show resolved Hide resolved
pkg/functionconfig/types.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/processor/trigger/http/trigger.go Outdated Show resolved Hide resolved
}

// use the event @ the worker index
// TODO: event already used?
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is not you TODO, but do you have an idea what does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomerShor No, but I can assume that it is only relevant for normal flow with single event processing, because in that case we use pre-defined events to wrap the context

pkg/processor/trigger/trigger.go Outdated Show resolved Hide resolved
@TomerShor TomerShor added the 1.14 PRs for version 1.14 label May 29, 2024
@TomerShor TomerShor merged commit ef7a014 into nuclio:development Jun 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants