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.

Sign up for GitHub

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

Add response code to common http_client #52

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add response code to common http_client #52

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
99f80db
Merge pull request #6 from fission/master
ranoble Feb 15, 2021
f57ba6a
Update Common to send a status code as part of the HTTP response. Thi…
ranoble Feb 15, 2021
3440890
Return response if the response is present, but it returns an error
ranoble Feb 15, 2021
6ef5e62
Changed the response code to a pointer, this allows us to return nil …
ranoble Feb 15, 2021
cefec02
Add better method docs to HandleHTTPRequest
ranoble Feb 15, 2021
c9042ae
Fix
NikhilSharmaWe Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,17 @@ func ParseConnectorMetadata() (ConnectorMetadata, error) {
return meta, nil
}

// HandleHTTPRequest sends message and headers data to HTTP endpoint using POST method and returns response on success or error in case of failure
func HandleHTTPRequest(message string, headers http.Header, data ConnectorMetadata, logger *zap.Logger) (*http.Response, error) {
// HandleHTTPRequest sends message and headers data to HTTP endpoint using POST method and returns the HTTP response code (pass or fail, but nil on failed request),
// the response (nil on failed request) and an error in case of failure in either request or in the server's handling of the request
func HandleHTTPRequest(message string, headers http.Header, data ConnectorMetadata, logger *zap.Logger) (*int, *http.Response, error) {

var resp *http.Response
for attempt := 0; attempt <= data.MaxRetries; attempt++ {
// Create request
req, err := http.NewRequest("POST", data.HTTPEndpoint, strings.NewReader(message))
if err != nil {
return nil, errors.Wrapf(err, "failed to create HTTP request to invoke function. http_endpoint: %v, source: %v", data.HTTPEndpoint, data.SourceName)
// Request not sent.
return nil, nil, errors.Wrapf(err, "failed to create HTTP request to invoke function. http_endpoint: %v, source: %v", data.HTTPEndpoint, data.SourceName)
}

// Add headers
Expand All @@ -83,18 +85,18 @@ func HandleHTTPRequest(message string, headers http.Header, data ConnectorMetada
}
if err == nil && resp.StatusCode >= 200 && resp.StatusCode < 300 {
// Success, quit retrying
return resp, nil
return &resp.StatusCode, resp, nil
}
}

if resp == nil {
return nil, fmt.Errorf("every function invocation retry failed; final retry gave empty response. http_endpoint: %v, source: %v", data.HTTPEndpoint, data.SourceName)
return nil, nil, fmt.Errorf("every function invocation retry failed; final retry gave empty response. http_endpoint: %v, source: %v", data.HTTPEndpoint, data.SourceName)
}

if resp.StatusCode < 200 || resp.StatusCode > 300 {
return nil, fmt.Errorf("request returned failure: %v. http_endpoint: %v, source: %v", resp.StatusCode, data.HTTPEndpoint, data.SourceName)
return &resp.StatusCode, resp, fmt.Errorf("request returned failure: %v. http_endpoint: %v, source: %v", resp.StatusCode, data.HTTPEndpoint, data.SourceName)
}
return resp, nil
return &resp.StatusCode, resp, nil
Copy link
Contributor

@therahulbhati therahulbhati Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. I would like to understand the reason why we are returning status code if it can be extracted from response.

Copy link
Member

@RealHarshThakur RealHarshThakur Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't be duplicate if we have status code , body and error as paramaters, right?

Copy link
Author

@ranoble ranoble Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

@RealHarshThakur RealHarshThakur Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@ranoble So do you suggest we should read the status code from the response object itself? That's the point of the PR though

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

It is also backwards compatible - and I always prefer that...

Copy link
Contributor

@therahulbhati therahulbhati Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@ranoble From the code changes you made, it seems you have introduced the following changes:

  1. Returning status code from function
  2. 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.

}

//GetAwsConfig get's the configuration required to connect to aws
Expand Down