Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix(batches): show warning instead of error when the changeset status is not FAILED#64243

Merged
bahrmichael merged 1 commit into
mainfrom
bahrmichael/batch-retry-warning-2
Aug 2, 2024
Merged

fix(batches): show warning instead of error when the changeset status is not FAILED#64243
bahrmichael merged 1 commit into
mainfrom
bahrmichael/batch-retry-warning-2

Conversation

@bahrmichael

@bahrmichael bahrmichael commented Aug 2, 2024

Copy link
Copy Markdown
Contributor

With SRCH-802 we saw error messages when batch changes may still retry and resolve the problem. To give a better distinction between resolvable and non-resolvable errors, I'm changing the colour variation from error to warning if the changeset has not been marked as FAILED yet.

Before:

Screenshot 2024-08-02 at 12 44 27

After:

Screenshot 2024-08-02 at 12 36 23

Test plan

Manual testing

Changelog

  • Batch changes that are still retrying now show a warning instead of an error.

@cla-bot cla-bot Bot added the cla-signed label Aug 2, 2024
@bahrmichael bahrmichael changed the title bahrmichael/batch-retry-warning-2 fix(batches): show warning instead of error when the changeset status is not FAILED Aug 2, 2024
@bahrmichael bahrmichael marked this pull request as ready for review August 2, 2024 10:55
@bahrmichael bahrmichael requested a review from a team August 2, 2024 11:21

@peterguy peterguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is a good idea. Do we collect these errors or will they be lost once the batch change is marked as failed?

@bahrmichael

Copy link
Copy Markdown
Contributor Author

I think this is a good idea. Do we collect these errors or will they be lost once the batch change is marked as failed?

The batch changes reconciler stores the latest error it has seen. If it fully fails, it will continue to display that error, and if it succeeds the errors may be cleaned up (not sure if the previous_error field gets cleaned up on success, but if it's successful it should be okay).

@bahrmichael bahrmichael merged commit 4a565a8 into main Aug 2, 2024
@bahrmichael bahrmichael deleted the bahrmichael/batch-retry-warning-2 branch August 2, 2024 14:22
@bahrmichael

Copy link
Copy Markdown
Contributor Author

Update: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/batches/reconciler/executor.go?L155 shows that the previous error message is deleted if the action that the worker should perform completes without an error. So if it fails, it should keep the previous error in the database.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants