-
Notifications
You must be signed in to change notification settings - Fork 89
Adding proto only Request Source Config factory #560
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.
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
Changes from all commits
99f45d1
075a4cf
ecbf407
8fb11b6
032dbba
953e953
6185edb
cded6ad
fbfe523
0d70179
cdb7efe
f15bb47
59359a2
f63df91
406bd6b
f0b697b
8425e48
fb4f372
c6eae6c
96ae38b
b17240f
5c5a146
29591ff
7d928a9
ff82d95
98381a4
83ac556
38c5084
fa4e285
8551c76
437de5e
6cef87e
c84ba4d
77d33f9
73aa686
ef77d65
b9182a0
e38dd80
21734e4
7cda9d2
f827aca
d194736
7820cf9
bee7805
3db68ce
c0ec4dd
a1dd3f2
0b25261
c3fa23c
547977e
c300203
8f4d4d9
5ae2d51
fe74b6d
6d87aea
ca7a0c2
967e3f8
14b7d79
c4985bb
922639a
81dfece
bf08661
34900c5
768fa81
e93bb58
f47cbd4
7c5c6b2
1b02cec
4d75602
c74f535
9949fb8
fcf10d3
fd9744d
68c1037
883ee30
8c34b25
6ce7e74
92927e5
a62f2d5
16f89c6
f4257e3
d075588
eb10a3b
cdbd6bf
17acc5f
55a0e62
fa23aad
b8e2447
248f439
91703b5
bbc0c2a
517c83d
c1d0428
bc86c09
33e3564
338a874
402ada2
017481f
3ed43fc
247b71b
171fb2e
5df83ed
0af3ffd
ff90e59
e57c05f
55244b0
c710d37
fbb2fdc
ae0f4a8
e092de0
19b7aa0
dae3098
a4b6fb1
55f685e
59c5b94
2ea0c5e
d40a073
7146079
7ab1132
72c23d0
2b1170c
9dc3c4b
b555a71
3924eee
75eca07
9c93ff9
e2af886
edae9be
949d731
59d490f
078329a
530cc9c
22cc3f8
32489d0
324bb23
5d6bb26
32ae13d
12708ac
eff2f4e
b890df4
a907913
49a47a4
b711028
8a2d792
22b93aa
7b9c175
bab49fc
fc7eda6
49e6eec
43de3d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // Implementations of RequestSourceConfigFactories that make a RequestOptionsListRequestSource. | ||
| // Implementations of RequestSourceConfigFactories that make a OptionsListRequestSource. | ||
| #pragma once | ||
|
|
||
| #include "envoy/registry/registry.h" | ||
|
|
@@ -25,14 +25,13 @@ namespace Nighthawk { | |
| // RequestGenerator produced by get() will use options from the options_list to overwrite values in | ||
| // the default header, and create new requests. if total_requests is greater than the length of | ||
| // options_list, it will loop. This is not thread safe. | ||
| class RequestOptionsListRequestSource : public RequestSource { | ||
| class OptionsListRequestSource : public RequestSource { | ||
| public: | ||
| RequestOptionsListRequestSource(const uint32_t total_requests, | ||
| Envoy::Http::RequestHeaderMapPtr header, | ||
| const nighthawk::client::RequestOptionsList& options_list); | ||
| OptionsListRequestSource(const uint32_t total_requests, Envoy::Http::RequestHeaderMapPtr header, | ||
| const nighthawk::client::RequestOptionsList& options_list); | ||
|
|
||
| // This get function is not thread safe, because multiple threads calling get simultaneously will | ||
| // result in a collision as it attempts to update its request_count_. | ||
| // result in a collision. | ||
| RequestGenerator get() override; | ||
|
|
||
| // default implementation | ||
|
|
@@ -45,40 +44,75 @@ class RequestOptionsListRequestSource : public RequestSource { | |
| const uint32_t total_requests_; | ||
| }; | ||
|
|
||
| // Factory that creates a RequestOptionsListRequestSource from a FileBasedPluginConfig proto. | ||
| // Registered as an Envoy plugin. | ||
| // Implementation of RequestSourceConfigFactory which produces a RequestSource that keeps an | ||
| // RequestOptionsList in memory, and loads it with the RequestOptions taken from a file. All plugins | ||
| // configuration are specified in the request_source_plugin.proto. This class is not thread-safe, | ||
| // because it loads its RequestOptionlist in memory from a file when first called. | ||
| // Usage: assume you are passed an appropriate Any type object called config, an Api object called | ||
| // api, and a default header called header. auto& config_factory = | ||
| // Factory that creates a OptionsListRequestSource from a FileBasedOptionsListRequestSourceConfig | ||
| // proto. Registered as an Envoy plugin. Implementation of RequestSourceConfigFactory which produces | ||
| // a RequestSource that keeps an RequestOptionsList in memory, and loads it with the RequestOptions | ||
| // taken from a file. All plugins configuration are specified in the request_source_plugin.proto. | ||
| // This class is thread-safe, | ||
| // Usage: assume you are passed an appropriate Any type object called config, an Api | ||
| // object called api, and a default header called header. auto& config_factory = | ||
| // Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>( | ||
| // "nighthawk.file-based-request-source-plugin"); | ||
| // RequestSourcePtr plugin = | ||
| // config_factory.createRequestSourcePlugin(config, std::move(api), std::move(header)); | ||
| class OptionsListFromFileRequestSourceFactory : public virtual RequestSourcePluginConfigFactory { | ||
| class FileBasedOptionsListRequestSourceFactory : public virtual RequestSourcePluginConfigFactory { | ||
| public: | ||
| std::string name() const override; | ||
|
|
||
| Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
|
|
||
| // This implementation is not thread safe. Only the first call to createRequestSourcePlugin will | ||
| // load the file from memory and subsequent calls just make a copy of the options_list that was | ||
| // already loaded. The OptionsListFromFileRequestSourceFactory will not work with multiple | ||
| // different files for this reason. | ||
| // This method will also error if the file can not be loaded correctly, e.g. the file is too large | ||
| // or could not be found. | ||
| // This implementation is thread safe. There is currently a behaviour such that only the first | ||
| // call to createRequestSourcePlugin will load the options list into memory and subsequent calls | ||
| // just make a copy of the options_list that was already loaded. The | ||
| // FileBasedOptionsListRequestSourceFactory will not work with multiple different files for this | ||
| // reason. | ||
| // TODO: This memory saving is likely a premature optimization, and should be removed. | ||
| // This method will also error if the | ||
| // file can not be loaded correctly, e.g. the file is too large or could not be found. | ||
| RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& message, | ||
| Envoy::Api::Api& api, | ||
| Envoy::Http::RequestHeaderMapPtr header) override; | ||
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable file_lock_; | ||
| nighthawk::client::RequestOptionsList options_list_; | ||
| absl::optional<nighthawk::client::RequestOptionsList> options_list_; | ||
| }; | ||
|
|
||
| // This factory will be activated through RequestSourceFactory in factories.h | ||
| DECLARE_FACTORY(OptionsListFromFileRequestSourceFactory); | ||
| DECLARE_FACTORY(FileBasedOptionsListRequestSourceFactory); | ||
|
|
||
| // Factory that creates a OptionsListRequestSource from a InLineOptionsListRequestSourceConfig | ||
| // proto. Registered as an Envoy plugin. Implementation of RequestSourceConfigFactory which produces | ||
| // a RequestSource that keeps an RequestOptionsList in memory, and loads it with the RequestOptions | ||
| // passed to it from the config. All plugins configuration are specified in the | ||
| // request_source_plugin.proto. | ||
| // This class is thread-safe, | ||
| // Usage: assume you are passed an appropriate Any type object called | ||
| // config, an Api object called api, and a default header called header. auto& config_factory = | ||
| // Envoy::Config::Utility::getAndCheckFactoryByName<RequestSourcePluginConfigFactory>( | ||
| // "nighthawk.in-line-options-list-request-source-plugin"); | ||
| // RequestSourcePtr plugin = | ||
| // config_factory.createRequestSourcePlugin(config, std::move(api), std::move(header)); | ||
|
|
||
| class InLineOptionsListRequestSourceFactory : public virtual RequestSourcePluginConfigFactory { | ||
| public: | ||
| std::string name() const override; | ||
| Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
|
|
||
| // This implementation is thread safe. There is currently a behaviour such that only the first | ||
| // call to createRequestSourcePlugin will load the options list into memory and subsequent calls | ||
| // just make a copy of the options_list that was already loaded. | ||
| // TODO: This memory saving is likely a premature optimization, and should be removed. | ||
| RequestSourcePtr createRequestSourcePlugin(const Envoy::Protobuf::Message& message, | ||
| Envoy::Api::Api& api, | ||
| Envoy::Http::RequestHeaderMapPtr header) override; | ||
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable options_list_lock_; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are saying the class is not thread safe, why do we need a lock? Once we proclaim it thread-unsafe, nobody should be calling it from multiple threads. If we do include a lock, we should use it to its intended purpose which is to make the class thread safe. Maybe I am misunderstanding it, but looks like we are in a hybrid state where we do pay the cost of locking for no benefit. Taking a step back, we should probably first agree whether we need this class to be thread safe. If you think we do, can you help by describing a scenario where this needs to be accessed by multiple threads? Once we understand that, we can discuss how to design it to be compatible with the intended use.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be thread safe. Sorry for the confusion. |
||
| absl::optional<nighthawk::client::RequestOptionsList> options_list_; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this field need to be optional? When would we create an instance of InLineOptionsListRequestSourceFactory without an options_list_?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this optional to indicate the state before and after having saved the options_list_. |
||
| }; | ||
|
|
||
| // This factory will be activated through RequestSourceFactory in factories.h | ||
| DECLARE_FACTORY(InLineOptionsListRequestSourceFactory); | ||
|
|
||
| } // namespace Nighthawk | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be
FileBasedPluginRequestSourceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this naming better expresses that they're both making an optionsListRequestSource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean isn't this a typo? This appears to be the comment above
FileBasedPluginConfigThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OHHH I didn't see
Filein the middle of the name, never mind!