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

EDU-3748: Tweak signal vs update text #3277

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison commented Jan 7, 2025

Suggested improvement to signal vs update description:

This was hard to understand:

The clients care more about the latency of sending the message than they do about the latency of the end-to-end operation.

Part of the problem is that no "end-to-end operation" is defined in the case of signal. We don't lose anything by deleting this since the update bullet points make the end-to-end latency point clearly:

The clients want a low-latency end-to-end operation and are willing to wait for it to finish or be validated.

I've replaced it with a bullet reminding users of the important point that they simply do not get a result/error in the case of signal. I think that this change results in bullet points that better communicate the appeal of update, and the continuining role for signal.

@dandavison dandavison requested a review from a team as a code owner January 7, 2025 14:03
@dandavison dandavison force-pushed the message-passing-tweak branch from 3994ed4 to 95e857b Compare January 7, 2025 14:13
@fairlydurable fairlydurable changed the title Tweak signal vs update text EDU-3748: Tweak signal vs update text Jan 7, 2025
@fairlydurable fairlydurable added cross-team This issue or PR was submitted from within Temporal tracking-internally labels Jan 7, 2025
@@ -54,7 +54,7 @@ Unlike Signals, Updates must be synchronous. That is, they must wait for the Wor
Use Signals instead of Updates when:

- The Workflow's clients want to quickly move on after sending an asynchronous message.
- The clients care more about the latency of sending the message than they do about the latency of the end-to-end operation.
- The clients don't need a result or exception from the message.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is fine, the first bullet point mostly covers what I was trying to say in the second point.
One optional note: "don't need a result or exception" is a negative, whereas the others are positive, which adds ambuigity to the logic (is it an or or an and?) and messes with the vibe that update is the default and you actively need a reason to choose signal.
I like being cautious about recommending signal because it swallows unexpected exceptions such as deserialization errors.
We could either cut this bullet point entirely or rephrase as something like "you don't need a result and are okay are willing to 'fire and forget'" to make it sound slightly scarier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I'm fine with whatever variant here. I was actually trying to do the same as you -- I thought what I wrote sounded offputting, since a result or exception is a desirable thing. I've changed it to

The clients are willing to "fire and forget": they don't want a result or exception from the message.

The negative "don't want ..." construction there matches the next bullet point about Signal ("don't want to rely on the Worker being available").

There was already a mixture of negative and positive constructions but I wouldn't worry about the coherency from a boolean logic POV; it'll be understood.

@fairlydurable
Copy link
Contributor

Hi @drewhoskins-temporal. Is this PR ready for review and merge? Thanks!

Copy link
Contributor

@fairlydurable fairlydurable left a comment

Choose a reason for hiding this comment

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

Tech review done.
No docs issues.

Unblocking to move forward.

@fairlydurable fairlydurable merged commit 367d6aa into main Jan 22, 2025
4 checks passed
@fairlydurable fairlydurable deleted the message-passing-tweak branch January 22, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team This issue or PR was submitted from within Temporal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants