Skip to content

+str Add startAfterNrOfConsumers to BroadcastHub.#275

Merged
He-Pin merged 1 commit intoapache:mainfrom
He-Pin:startAfterNrOfConsumers
Aug 5, 2023
Merged

+str Add startAfterNrOfConsumers to BroadcastHub.#275
He-Pin merged 1 commit intoapache:mainfrom
He-Pin:startAfterNrOfConsumers

Conversation

@He-Pin
Copy link
Member

@He-Pin He-Pin commented Mar 27, 2023

@He-Pin He-Pin force-pushed the startAfterNrOfConsumers branch from 2988558 to b97b38c Compare March 27, 2023 13:55
setKeepGoing(true)
callbackPromise.success(getAsyncCallback[HubEvent](onEvent))
pull(in)
if (startAfterNrOfConsumers == 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The old behavior

@He-Pin He-Pin added this to the 1.1.0 milestone Mar 27, 2023
@mdedetrich
Copy link
Contributor

So I had a quick look into this and it appears to be the same as the equivalent upstream akka PR and it was merged there so there shouldn't be any problems. Only question arising from #261 is whether for Pekko 1.1 BroadcastHub.sink should default to startAfterNrOfConsumers = 1.

Ill have a proper look in the next couple days.

@He-Pin
Copy link
Member Author

He-Pin commented Mar 27, 2023

@Aaronontheweb ping you too.

@mdedetrich Yes, I was thinking the same, should we make the default value to 1, which is a behavior change.

@He-Pin He-Pin closed this Mar 28, 2023
@He-Pin He-Pin reopened this Mar 28, 2023
@He-Pin
Copy link
Member Author

He-Pin commented Mar 28, 2023

Fail is #276
Not relevant.

@He-Pin He-Pin force-pushed the startAfterNrOfConsumers branch 2 times, most recently from 36cbd7c to ea6741d Compare August 4, 2023 05:20
@He-Pin He-Pin force-pushed the startAfterNrOfConsumers branch from ea6741d to b9fb22e Compare August 4, 2023 11:42
@He-Pin He-Pin requested a review from pjfanning August 4, 2023 11:42
Copy link
Member

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - but I would appreciate if someone else has a look too

@He-Pin
Copy link
Member Author

He-Pin commented Aug 4, 2023

@mdedetrich would you like give another look, thanks

@He-Pin
Copy link
Member Author

He-Pin commented Aug 5, 2023

I think this code is fine as it was merged into akka too. let me merge it to keep the pr queue shorter.

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.

3 participants