-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: panic due to mark retention task error #20137
Conversation
Signed-off-by: chlins <chenyuzh@vmware.com>
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.
lgtm
@@ -154,11 +154,11 @@ func (c *controller) Start(ctx context.Context, policy *replicationmodel.Policy, | |||
func (c *controller) markError(ctx context.Context, executionID int64, err error) { | |||
logger := log.GetLogger(ctx) | |||
// try to stop the execution first in case that some tasks are already created | |||
if err := c.execMgr.StopAndWait(ctx, executionID, 10*time.Second); err != nil { | |||
logger.Errorf("failed to stop the execution %d: %v", executionID, err) | |||
if stopErr := c.execMgr.StopAndWait(ctx, executionID, 10*time.Second); stopErr != nil { |
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.
Because the code change is related to execMgr, is it possible to extract a function and move it to execMgr?
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.
lgtm
Clarify err assignment to prevent panic caused by overwriting.
Thank you for contributing to Harbor!
Comprehensive Summary of your change
Issue being fixed
Fixes #20129
Please indicate you've done the following: