Adding proto only Request Source Config factory #560
Conversation
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
a global request generator. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Adding comments. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
…TestForRequestSourceFactory Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
request source. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
|
/retest |
|
🔨 rebuilding |
| // The pluginfactory will load the file located in file_path as long as it is below max_file_size, | ||
| // if it's too large it will throw an error. | ||
| // if it's too large it will throw an error. This field is optional with a default of 1000000. |
There was a problem hiding this comment.
(pre-existing) Can we mention the unit? Is it bytes?
| // Configuration for StubPluginRequestSource (plugin name: "nighthawk.stub-request-source-plugin") | ||
| // The plugin does nothing. This is for testing and comparison of the Request Source Plugin Factory | ||
| // mechanism using a minimal version of plugin that does not require a more complicated proto or | ||
| // file reading. | ||
| message StubPluginConfig { | ||
| // test input value which is the only output value in the headers produced from the | ||
| // requestGenerator for the StubRequestSource. | ||
| // requestGenerator for the StubRequestSource. This field is optional with a default of 0. |
There was a problem hiding this comment.
(nit) Consider removing the comment addition here, since it falls into the a comment category that just "describes the language". It is obvious from the definition below that the field is optional and what its default value is.
The style guide gives some advice on how to write good comments:
https://google.github.io/styleguide/cppguide.html#Comments
A good advice to follow is to avoid writing comments about obvious things, or comments that just describe the language syntax. We should assume that the reader of the comments is familiar with the syntax and just provide comments on tricky parts or logic.
| std::string name() const override; | ||
| Envoy::ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
|
|
||
| // This implementation is not thread safe. There is a race condition because only the first call |
There was a problem hiding this comment.
I believe we have discussed this in one of the previous PRs. We shouldn't explain why it isn't thread safe as that is an implementation comment. Also the comment whether a class is or isn't thread safe belongs at class level, not method level.
Let's remove all this and instead add a single sentence at the end of the class docstring:
// This class is not thread safe.
class InLineOptionsListRequestSourceFactory : public virtual {...}Edit after looking at the current implementation - the class actually may be thread safe, because it uses a lock around this call, so that doesn't seem to be a race. However let's first discuss what we need as noted in the other comment.
There was a problem hiding this comment.
I was trying to add the clarification because of a discussion I had with Eric on this CL. I think the issue may be that I shouldn't be storing the value in the factory itself. I'm a little uncertain on this one. Essentially the problem that has resulted is that by storing the value in the factory itself, although in practice we will always get the same proto passed in from the CLI to each of the workers and thus to each call of the RequestSourcePluginConfigFactory, that technically creates a condition wherein, if multiple calls are made to the same factory with different protos, only the first one is acknowledged. In practice, as I say, this doesn't matter, but in theory this behaviour seems like some kind of race condition.
There was a problem hiding this comment.
Thank you for the clarification, sounds like all we need to do is adjust the terminology. This class actually is thread-safe. Definition of race condition is clear - a code is racy when there are multiple threads accessing the same memory and at least one of them writes. Such situation isn't possible in this code, since we are using a lock. To address this - let's update the comment at the class level and say "This class is thread-safe".
As for the interesting behavior you are describing, for now we should just document it. Can you update the documentation so that it doesn't refer to a race condition and instead mentions that this function has a specific behavior on the first call? We can choose to remove the premature optimization in a later PR.
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable options_list_lock_; | ||
| absl::optional<nighthawk::client::RequestOptionsList> options_list_; |
There was a problem hiding this comment.
Does this field need to be optional? When would we create an instance of InLineOptionsListRequestSourceFactory without an options_list_?
There was a problem hiding this comment.
I made this optional to indicate the state before and after having saved the options_list_.
| Envoy::Http::RequestHeaderMapPtr header) override; | ||
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable options_list_lock_; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It should be thread safe. Sorry for the confusion.
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
mum4k
added
waiting-for-changes
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
wjuan-AFK
added
waiting-for-review
mum4k
added
waiting-for-changes
|
/retest |
|
🔨 rebuilding |
wjuan-AFK
added
waiting-for-review
Commit to add RequestSourcePlugin option to the CLI. base PR: #560. This is necessary to make use of the previous PRs adding the requestsourcepluginfactory via the CLI. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
This is a more usable version of the Request Source Config Factory similar to the FIle Based Config Factory which is easier to pass through the CLI.