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

Configurable batch sizes with the max_elements subscription option #185

Merged
merged 6 commits into from
Oct 26, 2024

Conversation

MrAnno
Copy link
Contributor

@MrAnno MrAnno commented Oct 24, 2024

This PR adds the max_elements option to subscription configurations, making it possible to control the maximum number of events the client aggregates.

For example, setting it to 1 allows real-time event flow, which makes debugging or the validation of an initial OpenWEC setup easier.

Config example:

# conf/my-test-subscription.toml
[options]
max_elements = 1

Implementation/testing notes:

  • I tested the database migration only with SQLite.
  • I haven't added the new option to the deprecated CLI interface.
  • I haven't added the new option to the v1 version of ImportExport (I'm not sure if this is right or not).

@vruello
Copy link
Contributor

vruello commented Oct 24, 2024

Thanks for this nice PR 👍

I need some time to review and test it, but it looks pretty complete at first sight.

The maximum number of events that the client should aggregate before
sending a batch.
@vruello
Copy link
Contributor

vruello commented Oct 26, 2024

I have added a check for max_elements in database tests. I also tested the feature on my lab, it works as expected 👍

I haven't added the new option to the v1 version of ImportExport (I'm not sure if this is right or not).

Since the default value of max_elements is None, there is no need to change the v1 import function. In addition, this change does not invalidate the previously generated v2 export files, so there is no need for a v3: editing the v2 schema as you did is fine.

Thank you very much!

@vruello vruello merged commit 53ee64f into cea-sec:main Oct 26, 2024
1 check passed
@MrAnno
Copy link
Contributor Author

MrAnno commented Oct 26, 2024

Thank you :)

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.

2 participants