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.

Sign up for GitHub

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

Jump to bottom

feat: added retry on files sync error #9261

Merged
merged 3 commits into from
Jul 12, 2024
Merged

feat: added retry on files sync error #9261

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65cfd43
feat: added retry on files sync error
idsulik Jan 18, 2024
15fabca
reverted code style changes
idsulik Jan 18, 2024
c09fa6c
fix(dev.go): change package order
idsulik Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions pkg/skaffold/runner/dev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"strconv"
"time"

"github.com/cenkalti/backoff/v4"

"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/event"
eventV2 "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/event/v2"
Expand Down Expand Up @@ -85,9 +87,13 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer) error {
r.changeSet.ResetSync()
r.intents.ResetSync()
}()

// todo: make this configurable
Copy link
Contributor

@ericzzzzzzz ericzzzzzzz Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Did you leave this here intentionally while working on this feature? Were you planning to incorporate any changes before committing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not in this PR, I left it in case if anyone wants to do it, maybe I'll do it by myself, but not now

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to implement configurable re-try for sync and default it to 0 -- not re-try to keep the user experience the same as before, if we want to have a sync retry, but I am still not convinced that we need this.

While // todo comments might seem like helpful reminders, they often create more problems than they solve:

Limited Visibility: Only those who stumble upon the specific code section see the comment, potentially leaving others unaware of the task.
Unclear Ownership and Status: There's no indication of who's responsible for the task, whether it's assigned, in progress, or forgotten.
Lack of Discussion: Meaningful conversations are difficult without a dedicated issue or pull request, hindering collaboration.
Prioritization Challenges: Maintainers struggle to assess the urgency and importance of tasks scattered throughout the codebase.
Ambiguity in Pull Requests: It's unclear if // todo marks unfinished work or instructions for future changes, causing confusion during reviews.
These issues often lead to // todo items being overlooked or forgotten.

A more effective approach is to create an issue for each task:

  • Issues provide a centralized location for tracking, assigning, and discussing tasks.
  • They allow for clear prioritization and status updates.
  • Discussions within issues create better communication and collaboration.

opts := backoff.WithMaxRetries(backoff.NewExponentialBackOff(), 3)
instrumentation.AddDevIteration("sync")
meterUpdated = true
for _, s := range r.changeSet.NeedsResync() {

syncHandler := func(s *sync.Item) error {
fileCount := len(s.Copy) + len(s.Delete)
output.Default.Fprintf(out, "Syncing %d files for %s\n", fileCount, s.Image)
fileSyncInProgress(fileCount, s.Image)
Expand All @@ -99,10 +105,23 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer) error {
eventV2.TaskFailed(constants.DevLoop, err)
endTrace(instrumentation.TraceEndError(err))

return nil
return err
}

fileSyncSucceeded(fileCount, s.Image)

return nil
}
for _, s := range r.changeSet.NeedsResync() {
err := backoff.Retry(
func() error {
return syncHandler(s)
}, backoff.WithContext(opts, childCtx),
)

if err != nil {
ericzzzzzzz marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
}
endTrace()
}
Expand Down
Loading