-
Notifications
You must be signed in to change notification settings - Fork 152
Conversation
⚠️ No Changeset found
Latest commit: 1aa1066
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop this if not used. Easier to add later if needed. Just thinking about the feature, this should be needed in the model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, addressed in eff1757.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also addressed in eff1757.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be a boolean, I think counter is also fine, but it may not be accurate always, but that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 3544490.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, when you say it may not always be accurate, do you mean it might take time to become consistent with the actual subscriber count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I think "not always accurate" is a bit of poor framing on my part. What I meant is that if the room is spread across many regions, inter-region messages have to propagate to the publisher node indicating interest from remote nodes and then back to the publisher. I say "poor framing" because that notification has to traverse the network. So, there is no magic to instantly get the subscriber count. In a single node case, it is instantaneous, so it will look right all the time. That's what I meant by not always accurate if it is not from a single node room.
The other point about using boolean is that you just have to send it when count becomes non-zero or zero. But, if you are using count, you have to technically send it anytime it changes. Again, that does not make it inaccurate, but inefficient for the purposes of dynacast, i. e. too many notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the info!
Closes BOT-446