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

Fix errcheck in service/worker/ #3731

Merged
merged 1 commit into from
Dec 21, 2022
Merged

Conversation

MichaelSnowden
Copy link
Contributor

What changed?
We now surface errors from code within service/worker.

  1. We log an error when we fail to unregister a membership listener in the per-namespace worker
  2. We log a fatal error when we fail to register the membership listener during startup
  3. We log a fatal error whenever any SDK workers fail to start
  4. The scheduler will retry now whenever the side effect of creating a new UUID fails (this should be rare, but we still shouldn't ignore these errors).

Why?
I made these changes so that we don't silently swallow these errors. Additionally, this will enable us to run errcheck on this directory.

How did you test it?
I tested this using the CI. However, I made sure that the not-so-important errors, like unregistering a membership listener, are only logged, not treated as fatal.

Potential risks
This could cause errors on startup/shutdown that users aren't used to.

Is hotfix candidate?
Let's note it because OSS users might see a new error and wonder why.

@MichaelSnowden MichaelSnowden requested a review from a team as a code owner December 20, 2022 00:57
@MichaelSnowden MichaelSnowden merged commit 1454f84 into master Dec 21, 2022
@MichaelSnowden MichaelSnowden deleted the errcheck/service/worker branch December 21, 2022 20:21
for {
b, err := s.processBuffer()
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the effort but this is wrong: returning an error here will end the workflow. Errors from terminate or cancel are supposed to be swallowed since they'll be retried at the next action attempt.

And most of the other changes in this file are just unnecessary clutter for type errors that can't ever happen.

Can we revert at least the changes to this file?

dnr added a commit to dnr/temporal that referenced this pull request Jan 7, 2023
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