admin: add API for creating chunked handlers#19831
Merged
jmarantz merged 9 commits intoenvoyproxy:mainfrom Feb 11, 2022
Merged
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
jmarantz
commented
Feb 10, 2022
| MAKE_ADMIN_HANDLER(runtime_handler_.handlerRuntimeModify), false, true}, | ||
| {"/reopen_logs", "reopen access logs", | ||
| MAKE_ADMIN_HANDLER(logs_handler_.handlerReopenLogs), false, true}, | ||
| makeHandler("/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false), |
Contributor
Author
There was a problem hiding this comment.
note this block of code is mechanistically transformed from its previous state, as we needed to add Handler wrappers to all the legacy callback-based handlers.
In a future PR, /stats and likely also /cluster_info will be converted to Chunked handlers.
…not have flow-control. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Contributor
Author
|
@krajshiva FYI |
zuercher
approved these changes
Feb 11, 2022
Member
zuercher
left a comment
There was a problem hiding this comment.
Looks good. Couple small comments, but they could be addressed in one of your follow-up PRs, if you'd like.
| Http::ResponseHeaderMap& response_headers, | ||
| Buffer::Instance& response, AdminStream& admin_stream) { | ||
| HandlerPtr handler = findHandler(path_and_query, admin_stream); | ||
| Http::Code code = handler->start(/*path_and_query, */ response_headers); |
Member
There was a problem hiding this comment.
Remove the path_and_query comment?
| RELEASE_ASSERT(request_headers_, ""); | ||
| Http::Code code = admin_server_callback_func_(path, *header_map, response, *this); | ||
| Admin::HandlerPtr handler = admin_handler_fn_(path, *this); | ||
| Http::Code code = handler->start(/*path, */ *header_map); |
| * Adds the next chunk of data to the response. Note that nextChunk can | ||
| * return 'true' but not add any data to the response, in which case a chunk | ||
| * is not sent, and a subsequent call to nextChunk can be made later, | ||
| * possibly after a post() or low-watermark callback on the http filter. |
Member
There was a problem hiding this comment.
I gather this is only true once #19898 lands and until then we spin calling nextChunk. I think it's ok to leave as long as you expect that PR to land soon-ish.
Contributor
Author
|
Thanks -- that's right on all counts and I'll clean up the comments in the next PR. I think the next PR is working but I can't prove it yet; I think I need a test using HttpAsyncHandler or something. |
joshperry
pushed a commit
to joshperry/envoy
that referenced
this pull request
Feb 13, 2022
Commit Message: The admin handler structure uses a single callback that is expected to fill in the entire response body, making it impossible to stream data out as it is generated. This PR refactors the internal admin structures to require a Handler object with a nextChunk method, to enable chunking. It defers to other PRs to * actually convert any handlers to use chunking (e.g. WiP stats: Reimplement existing /stats API in a way that can be chunked. envoyproxy#19693) * implement proper flow control in the AdminFilter to avoid generating chunks before clients are ready to receive them (WiP admin: add flow control to admin filter envoyproxy#19898) Additional Description: Risk Level: medium -- this is a pure refactor but not a trivial one. The same exact admin content should be generated before/after this change Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Josh Perry <josh.perry@mx.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: The admin handler structure uses a single callback that is expected to fill in the entire response body, making it impossible to stream data out as it is generated. This PR refactors the internal admin structures to require a Handler object with a nextChunk method, to enable chunking. It defers to other PRs to
Additional Description:
Risk Level: medium -- this is a pure refactor but not a trivial one. The same exact admin content should be generated before/after this change
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a