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

Make WalkDir return errors #19677

Merged
merged 9 commits into from
May 6, 2024
Merged

Conversation

klauspost
Copy link
Contributor

Description

This adds errors to WalkDir responses. This means that batch jobs now has an idea of whether they have failed prematurely. This is because if WalkDir terminates it could only close the channel, so it was impossible to detect if there were no more entries or an error occured.

I tried to adapt retry mechanisms reasonably - meaning what I could do without a bigger rewrite.

We now stop updating RetryAttempts since some of them were racy. Instead we add "Attempts" which counts total attempts.

We also stop counting failed retries as multiple errors. And if a retry succeeds it will deduct from the failures.

Please review carefully as I am not too deep into these systems.

How to test this PR?

Batch jobs with unstable network I guess.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

'opts.Marker` is causing many missed entries if used since results are returned unsorted. Also since pools are serialized.

Switch to do fully concurrent listing and merging across pools to return sorted entries.

Returning errors on listings is impossible with the current API, so document that.

Return an error at once if no drives are found instead of just returning an empty listing and no error.
# Conflicts:
#	cmd/erasure-server-pool.go
@klauspost klauspost mentioned this pull request May 6, 2024
8 tasks
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Few comments

cmd/batch-handlers.go Outdated Show resolved Hide resolved
cmd/batch-handlers.go Show resolved Hide resolved
Copy link
Contributor

@poornas poornas left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit 847ee5a into minio:master May 6, 2024
20 checks passed
@klauspost klauspost deleted the walk-return-errors branch May 28, 2024 15:09
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.

None yet

4 participants