Add response code to common http_client #52
Conversation
Catch Up
…s, and the inclusion of the request allows us to act according to requests that couldn't be sent, and ones that we're sent, and an attempt made at handling them, but failed.
…if we don't have a response code
|
Thanks for the PR @ranoble . We'll merge this and then we can follow it up with a PR where other packages use the latest git commit of it |
common/util.go
Outdated
| } | ||
| return resp, nil | ||
| return &resp.StatusCode, resp, nil |
There was a problem hiding this comment.
Thanks for the PR. I would like to understand the reason why we are returning status code if it can be extracted from response.
There was a problem hiding this comment.
I was thinking we should have status code, response body(instead of whole response) and error . If we look at how connectors are using it, it made sense to me if we do it that way. What are your thoughts?
There was a problem hiding this comment.
I agree we should utilize status code, but the same status code can be read from the response that way it's more pragmatic and avoid duplication. Think of this package as an HTTP package which only returns response and error.
There was a problem hiding this comment.
It wouldn't be duplicate if we have status code , body and error as paramaters, right?
There was a problem hiding this comment.
The core reason for this change was to allow an error handler to read the response headers, not just the body in the case of an error. This is initially to support keys for kafka in error topics - so that we can control partitioning and compaction.
We have 3 states:
- Request could not. be sent (network down etc)
- Request sent, but responded with an error - in this case with the current setup we get a nil for response.
- Success.
If we move to 'body' only, we'll lose a chance to handle partitioning and compaction or any of the more advanced features of kafka (at least for errors).
There was a problem hiding this comment.
@ranoble So do you suggest we should read the status code from the response object itself? That's the point of the PR though
There was a problem hiding this comment.
That was the initial proposal. The concern was that you'd have both a response AND an error.
I don't really see this as a problem (but then I'm not a golang native, so I'd follow your guidance), as you can safely ignore the response if you don't need it, but use it if you do (in the kafka case).
There was a problem hiding this comment.
It is also backwards compatible - and I always prefer that...
There was a problem hiding this comment.
@ranoble From the code changes you made, it seems you have introduced the following changes:
- Returning status code from function
- At line no. 97 you are returning resp instead of nil
if resp.StatusCode < 200 || resp.StatusCode > 300
The second change makes sense as we would have more info about errors which can be utilized while writing to error topic/queue. I don't think the first change is necessary as we can get the status code from response directly.
|
@vishal-biyani I am wondering given the dependency issue other packages seem to be running into for such a small piece of code, would it best if each connector had it's own implementation? In a way, only the aws connectors require the common package. Others connectors can have the same piece of code. |
|
@RealHarshThakur That works - what is change needed in this PR - or we need another set of PR for this one? |
This PR modified the HTTP client so that:
This means that we can act on a failed response, and that context is not lost.