MakeBucket: Retry with correct region #665
Conversation
harshavardhana
changed the title
vadmeste
force-pushed
the
error_resp_has_headers
branch
from
f903f0d to
e519f9f
Compare
vadmeste
changed the title
vadmeste
force-pushed
the
error_resp_has_headers
branch
from
e519f9f to
cadf71b
Compare
If MakeBucket is called with the wrong region, fetch the server's correct region from the S3 API response headers and retries sending the create bucket request with the new region.
vadmeste
force-pushed
the
error_resp_has_headers
branch
from
cadf71b to
864e52b
Compare
vadmeste
changed the title
|
Can this PR be reviewed again ? |
api.go
Outdated
| // Bucket region if set in error response and the error code dictates invalid region, | ||
| // we can retry the request with the new region. | ||
| if errResponse.Code == "InvalidRegion" && errResponse.Region != "" { | ||
| c.bucketLocCache.Set(metadata.bucketName, errResponse.Region) | ||
| if errResponse.Code == "InvalidRegion" && errResponse.Headers.Get("x-amz-bucket-region") != "" { |
There was a problem hiding this comment.
Don't do this, set the Region properly and remove Headers from errorResponse structure. Let me send a PR to this PR.
There was a problem hiding this comment.
There is a chance for this to go into an infinite loop, we just want to do this once.
There was a problem hiding this comment.
Don't do this, set the Region properly and remove Headers from errorResponse structure.
My aim was to make the code reflects exactly the spec even if it looks more longer, the reason is that the code will be easier to read and be compared with the S3 spec. Also we are not the creator of the spec and any optimization could bring new problems.
There was a problem hiding this comment.
Region is there because it does exist in some operations on AWS S3, so we can re-purpose making it more cleaner and use it as part of region retry. I sent a PR which retains Headers but extracts the value and sets it properly for errorResponse instead of spilling the code.
More changes to makeBucket retry.
| @@ -801,7 +801,7 @@ func TestRemovePartiallyUploaded(t *testing.T) { | |||
| if err == nil { | |||
| t.Fatal("Error: PutObject should fail.") | |||
| } | |||
| if err.Error() != "Proactively closed to be verified later." { | |||
| if err.Error() != "proactively closed to be verified later" { | |||
There was a problem hiding this comment.
Where did this change @vadmeste ?
There was a problem hiding this comment.
Test was failing without this change, 'proactively closed to be verified later' message should be the same as returned by the go routine created just above.
api.go
Outdated
|
|
||
| // Save the body. | ||
| errBodySeeker = bytes.NewReader(errBodyBytes) | ||
| res.Body = ioutil.NopCloser(errBodySeeker) |
There was a problem hiding this comment.
Is this needed? there seems like something similar is done above.
There was a problem hiding this comment.
Is this needed? there seems like something similar is done above.
Yes, this is needed. You can see just above that httpRespToErrorResponse() is reading from res.Body so this change puts back res.Body
There was a problem hiding this comment.
Yes but this is wrong see below
// Save the body back again.
errBodySeeker.Seek(0, 0) // Seek back to starting point.
res.Body = ioutil.NopCloser(errBodySeeker)
We seek back.
api.go
Outdated
| // China region. | ||
| location = "cn-north-1" | ||
| } | ||
| if metadata.bucketLocation == "" { |
There was a problem hiding this comment.
Is this necessary anymore?
There was a problem hiding this comment.
Is this necessary anymore?
No, we don't need this anymore since you brought back makeBucketRequest(), I'll update
vadmeste
force-pushed
the
error_resp_has_headers
branch
from
12133d7 to
15837c2
Compare
vadmeste
force-pushed
the
error_resp_has_headers
branch
from
15837c2 to
f302102
Compare
The main purpose if this PR is to be able to find x-amz-bucket-region
when S3 server returns InvalidRegion error. That would help clients
to retry creating bucket with the correct region.