Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
6 changes: 6 additions & 0 deletions include/envoy/buffer/buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ class Instance {
*/
virtual ssize_t search(const void* data, uint64_t size, size_t start) const PURE;

/**
* Construct a flattened string from a buffer.
* @return the flattened string.
*/
virtual std::string toString() const PURE;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: this method was previously implemented as a test utility; I made it slightly faster by right-sizing the string buffer first, and moving it into source/...


/**
* Write the buffer out to a file descriptor.
* @param fd supplies the descriptor to write to.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,8 @@ envoy_cc_library(
name = "protocol_interface",
hdrs = ["protocol.h"],
)

envoy_cc_library(
name = "query_params_interface",
hdrs = ["query_params.h"],
)
16 changes: 16 additions & 0 deletions include/envoy/http/query_params.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#include <map>
#include <string>

namespace Envoy {
namespace Http {
namespace Utility {

// TODO(jmarantz): this should probably be a proper class, with methods to serialize
// using proper formatting. Perhaps similar to
// https://github.com/apache/incubator-pagespeed-mod/blob/master/pagespeed/kernel/http/query_params.h

typedef std::map<std::string, std::string> QueryParams;

} // namespace Utility
} // namespace Http
} // namespace Envoy
1 change: 1 addition & 0 deletions include/envoy/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ envoy_cc_library(
":options_interface",
"//include/envoy/access_log:access_log_interface",
"//include/envoy/api:api_interface",
"//include/envoy/http:query_params_interface",
"//include/envoy/init:init_interface",
"//include/envoy/local_info:local_info_interface",
"//include/envoy/ratelimit:ratelimit_interface",
Expand Down
1 change: 1 addition & 0 deletions include/envoy/server/admin.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class AdminStream {
*/
virtual const Http::HeaderMap& getRequestHeaders() const PURE;
};

/**
* This macro is used to add handlers to the Admin HTTP Endpoint. It builds
* a callback that executes X when the specified admin handler is hit. This macro can be
Expand Down
18 changes: 18 additions & 0 deletions include/envoy/server/instance.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "envoy/access_log/access_log.h"
#include "envoy/api/api.h"
#include "envoy/http/query_params.h"
#include "envoy/init/init.h"
#include "envoy/local_info/local_info.h"
#include "envoy/network/listen_socket.h"
Expand Down Expand Up @@ -184,6 +185,23 @@ class Instance {
* @return the flush interval of stats sinks.
*/
virtual std::chrono::milliseconds statsFlushInterval() const PURE;

/**
* Defines a callback that is run when an admin request completes..
* The first parameter to the function is the response-headers, which
* will generally have Content-Type and Status fields.
*/
typedef std::function<void(Http::Code, Http::HeaderMap&, absl::string_view)> AdminResponseHandler;

/**
* Admin requests are generally handled by posting to the 'main' thread, so
* a callback must be supplied which will be called when the response is handled.
*
* @param path The admin request URL path as a string.
* @param params map of parameter names and values.
*/
virtual void adminRequest(absl::string_view path, const Http::Utility::QueryParams& params,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The server instance already exposes an Admin object. Seems like this isn't needed and should actually get routed through the admin object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point, that shouldn simplify the PR. Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It turns out not to dramatically simplify the PR as I just pushed the mocking boilerplate out of server and into admin :)

absl::string_view method, AdminResponseHandler& response_handler) PURE;
};

} // namespace Server
Expand Down
17 changes: 17 additions & 0 deletions source/common/buffer/buffer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -183,5 +183,22 @@ OwnedImpl::OwnedImpl(const Instance& data) : OwnedImpl() { add(data); }

OwnedImpl::OwnedImpl(const void* data, uint64_t size) : OwnedImpl() { add(data, size); }

std::string OwnedImpl::toString() const {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this different to linearize()? Do we need both variants?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I looked at linearize and though they seem similar, I don't see how either can be efficiently implemented on the other.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

well, you could linearize and then create a std;;string with the now linearized buffer. This would involve two copies, but is still O(n) and might be fine for the applications we would use this for, with the assumption being that you don't want to do toString() or linearize for anything performance sensitive anyway. There's also the option of just doing an absl::string_view around the linearized buffer.

@jmarantz jmarantz Jun 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

linearize mutates the buffer IIUC; I thought it made sense to have a simple observer that can return the fully flattened string.

Moreover it exists in the codebase now, in a test helper, and I'm mostly just moving it. However per your other comment -- I don't really need this in the interface, as everywhere in tests and in admin.cc where it's needed, I have an OwnedImpl. Removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think mutating is a benefit here, it's kind of like defragging. At least naively, it seems you should be able to implement it about as efficiently as the toString() operation.

uint64_t num_slices = getRawSlices(nullptr, 0);
RawSlice slices[num_slices];
getRawSlices(slices, num_slices);
size_t len = 0;
for (RawSlice& slice : slices) {
len += slice.len_;
}
std::string output;
output.reserve(len);
for (RawSlice& slice : slices) {
output.append(static_cast<const char*>(slice.mem_), slice.len_);
}

return output;
}

} // namespace Buffer
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/buffer/buffer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class OwnedImpl : public LibEventInstance {
ssize_t search(const void* data, uint64_t size, size_t start) const override;
int write(int fd) override;
void postProcess() override {}
std::string toString() const override;

Event::Libevent::BufferPtr& buffer() override { return buffer_; }

Expand Down
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ envoy_cc_library(
"//include/envoy/http:codes_interface",
"//include/envoy/http:filter_interface",
"//include/envoy/http:header_map_interface",
"//include/envoy/http:query_params_interface",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
Expand Down
Loading