Add optional targetClientId to ISignalMessage#19555
Merged
WillieHabi merged 11 commits intomicrosoft:mainfrom Feb 14, 2024
Merged
Add optional targetClientId to ISignalMessage#19555WillieHabi merged 11 commits intomicrosoft:mainfrom
WillieHabi merged 11 commits intomicrosoft:mainfrom
Conversation
github-actions
bot
added
public api change
WillieHabi
requested review from
Abe27342,
ChumpChief,
DLehenbauer,
Josmithr,
anthony-murphy,
jason-ha and
noencke
and removed request for
Abe27342,
ChumpChief,
DLehenbauer,
Josmithr,
anthony-murphy and
noencke
WillieHabi
requested review from
Abe27342,
ChumpChief,
DLehenbauer,
Josmithr,
anthony-murphy and
noencke
jason-ha
reviewed
Feb 8, 2024
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
jason-ha
reviewed
Feb 9, 2024
jason-ha
reviewed
Feb 9, 2024
jason-ha
requested changes
Feb 9, 2024
Contributor
jason-ha
left a comment
jason-ha
left a comment
There was a problem hiding this comment.
We should spruce up the comments. See some prior comments that are not yet Resolved.
(submit_signals_v2 reference doesn't belong on ISent; the base comments should reflect the bidi nature)
Contributor
Author
|
Made minor changes to the comments. Are these sufficient or was there something else in mind? |
jason-ha
reviewed
Feb 12, 2024
jason-ha
approved these changes
Feb 14, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updating ISignalMessage to inlcude optional targetSignalid.
This can be useful for receiving clients trying to understand the overall approximate load on service and maybe other understanding. If received signal has that property, then we know it wasn't broadcast to everyone.
FF API Council Context
I have a minor API change related to ISignalMessage. ODSP had made changes to allow for targeted signals and I have been tasked with doing some of the follow-up work related to this. Initially the idea was for the server to strip the targetClientId before sending the signal message to the targeted client, however Jason and Gary came to an agreement that it would be beneficial to leave this information on the message, which would involve adding an optional targetClientId member to ISignalMessage(Base). This could be useful for receiving clients trying to understand the overall approximate load on service and maybe other understanding. If received signal has that property, then we know it wasn't broadcast to everyone. Thanks.