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

Remove Zeek's batching of Broker messages #771

Open
jsiwek opened this issue Feb 3, 2020 · 7 comments
Open

Remove Zeek's batching of Broker messages #771

jsiwek opened this issue Feb 3, 2020 · 7 comments

Comments

@jsiwek
Copy link
Contributor

jsiwek commented Feb 3, 2020

Initial investigations to remove Zeek-side batching were done in these PRs:

But findings indicate those changes may want to wait and re-evaluate after migrating to CAF 0.18 which aims to decrease cost of sending many smaller messages and avoid performance drop currently seen with these changes.

@jsiwek jsiwek added this to the 3.2.0 milestone Feb 3, 2020
@jsiwek jsiwek added this to Unassigned / Todo in Release 3.2.0 via automation Feb 3, 2020
@jsiwek jsiwek moved this from Unassigned / Todo to Assigned / In Progress in Release 3.2.0 Feb 3, 2020
@jsiwek jsiwek added this to Unassigned / Todo in Release 4.0.0 via automation Jun 30, 2020
@jsiwek jsiwek removed this from Assigned / In Progress in Release 3.2.0 Jun 30, 2020
@jsiwek jsiwek modified the milestones: 3.2.0, 4.0.0 Jun 30, 2020
@jsiwek jsiwek moved this from Unassigned / Todo to Assigned / In Progress in Release 4.0.0 Oct 13, 2020
@rsmmr
Copy link
Member

rsmmr commented Dec 2, 2020

Looks like we're running out of time here, so moving to 4.1.0.

@rsmmr rsmmr modified the milestones: 4.0.0, 4.1.0 Dec 2, 2020
@jsiwek jsiwek added this to Unassigned / Todo in Release 4.1.0 via automation Dec 9, 2020
@jsiwek jsiwek removed this from Assigned / In Progress in Release 4.0.0 Dec 9, 2020
@jsiwek jsiwek moved this from Unassigned / Todo to Assigned / In Progress in Release 4.1.0 Mar 30, 2021
@rsmmr
Copy link
Member

rsmmr commented Jun 22, 2021

I think this is something to revisit once we have the new network protocol code landed in Broker.

@rsmmr rsmmr modified the milestones: 4.1.0, 4.2.0 Jun 22, 2021
@rsmmr rsmmr removed this from Assigned / In Progress in Release 4.1.0 Jun 22, 2021
@rsmmr rsmmr added this to Unassigned / Todo in Release 4.2.0 via automation Jun 22, 2021
@timwoj timwoj removed this from Unassigned / Todo in Release 4.2.0 Jun 24, 2021
@timwoj timwoj added this to Unassigned / Todo in Release 4.2.0 via automation Jun 24, 2021
@timwoj timwoj removed this from Unassigned / Todo in Release 4.2.0 Dec 9, 2021
@timwoj timwoj removed this from the 4.2.0 milestone Dec 9, 2021
@timwoj
Copy link
Contributor

timwoj commented Jun 24, 2022

@rsmmr @Neverlord Have enough of the changes landed in Broker now to revisit this?

@Neverlord
Copy link
Member

I believe so. I believe I did some benchmarking for this (with and without batching), probably a good idea to revisit that now.

@timwoj
Copy link
Contributor

timwoj commented Jul 14, 2022

I'll throw it in the 5.1 project. Hopefully we can get it closed out.

@timwoj timwoj added this to the 5.1.0 milestone Jul 14, 2022
@rsmmr
Copy link
Member

rsmmr commented Jul 14, 2022

Note that this comes with a risk of a major performance degradation in case that it doesn't quite work yet under heavy load. I would suggest we either do more testing first with the testing group before we merge, or we postpone for now in the spirit of not making large Broker changes this cycle.

@timwoj
Copy link
Contributor

timwoj commented Jul 14, 2022

Ah yah, forgot about that. Taking it back out of the 5.1 project. We'll revisit it in 5.2.

@timwoj timwoj removed this from the 5.1.0 milestone Jul 14, 2022
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

4 participants