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

Check for graveler *and* catalog ErrNotFound #4436

Merged
merged 2 commits into from
Oct 23, 2022

Conversation

arielshaqed
Copy link
Contributor

@arielshaqed arielshaqed commented Oct 23, 2022

Fixes #4427.

Opened #4435 to refactor to help protect against similar recurrences.

@arielshaqed arielshaqed added bug Something isn't working area/gateway Changes to the gateway include-changelog PR description should be included in next release changelog labels Oct 23, 2022
@arielshaqed
Copy link
Contributor Author

Tested manually:

❯ AWS_MAX_ATTEMPTS=1 aws --profile local-s3 --endpoint http://localhost:8000/ s3api get-object --bucket foo --key main/asdf -

An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist.

(also tested with a bad branch and a bad repo).

Will add separately unit tests for getobject and headobject.

Fixes #4427.

Opened #4435 to refactor to help protect against similar reccurrences.
@arielshaqed arielshaqed force-pushed the bug/4427-s3-get-object-500-not-found branch 2 times, most recently from d0fc8c1 to 2c9ef54 Compare October 23, 2022 09:09
Comment on lines 163 to 177
t.Run("Doesn't exist", func(t *testing.T) {
_, err := client.GetObject(ctx, repo, goodPath, minio.GetObjectOptions{})
if err != nil {
t.Errorf("client.GetObject(%s, %s): %s", repo, goodPath, err)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this test check for the correct response if the path doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, now :-)

@arielshaqed arielshaqed force-pushed the bug/4427-s3-get-object-500-not-found branch from 2c9ef54 to 7c8e5c8 Compare October 23, 2022 11:24
@arielshaqed arielshaqed force-pushed the bug/4427-s3-get-object-500-not-found branch from 7c8e5c8 to 946cf7b Compare October 23, 2022 11:26
Copy link
Contributor

@Jonathan-Rosenberg Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@arielshaqed
Copy link
Contributor Author

Thanks!

@arielshaqed arielshaqed merged commit 22bb945 into master Oct 23, 2022
@arielshaqed arielshaqed deleted the bug/4427-s3-get-object-500-not-found branch October 23, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway Changes to the gateway bug Something isn't working include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakeFS returns code 500 when getting absent object
3 participants