return an error when max retry is reached in executeMethod()

[ericychoi]

DO NOT MERGE, missing tests.

[harshavardhana]

This is not a bad idea but i think we can return the actual error rather than sending special error like retry. Right now it is sent it is just that DeleteObject() ignores it. You can modify that part of code instead.

[ericychoi]

I believe err is not sent back to RemoveObject(). Since c.do() will not return an error on 500. (https://github.com/minio/minio-go/blob/master/api.go#L479)

It will return an error on closed connection or empty response, but not on a valid 500.

Now I can inspect resp in RemoveObject() and if it contains non-2xx status code, return an error to signify that to the client. Would you think that's better? Keep in mind then the same issue will keep happening for any other calls that use executeMethod() (update, for instance)

[harshavardhana]

I believe err is not sent back to RemoveObject(). Since c.do() will not return an error on 500. (https://github.com/minio/minio-go/blob/master/api.go#L479)

It will return an error on closed connection or empty response, but not on a valid 500.

Now I can inspect resp in RemoveObject() and if it contains non-2xx status code, return an error to signify that to the client. Would you think that's better? Keep in mind then the same issue will keep happening for any other calls that use executeMethod() (update, for instance)

Yes in case of executeMethod() we should save the error regardless of the response codes.

[dtjm]

I think it's better to declare the error once at the package level, and return that instead.

On the other hand, in this case, would you want to return the last error received? I was thinking if you just return "Max retry reached", the caller won't have any idea what the underlying error was.

[harshavardhana]

Yes that is what we need here, we should only return the last error after all the retries.

[ericychoi]

As I said above, the last error is nil in this case, since c.Do() doesn't return error on 500.

I added an additional check here to make it clear (err == nil). I verified that this fixes my issue.

I can make another PR to attempt to fix this in RemoveObject(), but the point still stands this will keep happening for any other calls other than RemoveObject().

[ericychoi]

Created #555 as an alternative approach.

[harshavardhana]

You can close this @ericychoi

[harshavardhana]

This approach is no longer needed. Thanks @ericychoi