Skip to content

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Nov 16, 2021

While working on #62, I found that it was awkward to find a way to pass the channel signaling that the function completed to the router handler. In defining the handlers as they are in this PR, we can easily pass the channel to the handler function, without having to keep a reference to it somewhere.

@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Nov 16, 2021
@ghost
Copy link

ghost commented Nov 16, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-18T12:35:20.093+0000

  • Duration: 7 min 3 sec

  • Commit: 1d49a36

Test stats 🧪

Test Results
Failed 0
Passed 54
Skipped 0
Total 54

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@estolfo estolfo requested a review from astorm November 18, 2021 12:35
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

Looks like a welcome change -- have a routing system HTTP request multiplexer in place seems like a standard practice and should let us expand the capabilities of the extension further if needed.

No objections to landing this as but so approving, but I do have some small suggestion/questions below.

@estolfo estolfo merged commit 8e47191 into main Nov 19, 2021
@estolfo estolfo deleted the estolfo/servemux branch November 19, 2021 08:51
Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

couple comments but that's all, looks good!

return
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
Copy link
Contributor

Choose a reason for hiding this comment

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

The body shouldn't be massive in size, but a good habit is to operate on the resp.Body as an io.Reader, and use something like io.Copy to keep memory allocation down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I'll change that 👍🏼

log.Println("Could not read bytes from agent request body")
return
}
if r.Body == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

did you encounter the body being nil? I took a look at the docs because I was checking to see if you need to close the request body, and it says:

	// For server requests, the Request Body is always non-nil
	// but will return EOF immediately when no body is present.
	// The Server will close the request body. The ServeHTTP
	// Handler does not need to.

so I'm not sure if this nil check is required. I suppose it doesn't hurt, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the docs say it will never be nil, it doesn't seem necessary. I'll remove it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws-λ-extension AWS Lambda Extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants