Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adaptive load helper mocks #529

Merged
merged 53 commits into from
Sep 14, 2020
Merged

Adaptive load helper mocks #529

merged 53 commits into from
Sep 14, 2020

Conversation

eric846
Copy link
Contributor

@eric846 eric846 commented Sep 12, 2020 •
edited
Loading

  • MockNighthawkServiceClient
  • MockMetricsEvaluator
  • MockAdaptiveLoadSessionSpecProtoHelper

Part 8 of splitting PR #483.

eric846 added 30 commits June 1, 2020 17:23
@eric846
Merge pull request #5 from envoyproxy/master
8ea442d 
update from master
@eric846
Merge pull request #6 from envoyproxy/master
5ac755a 
merge from upstream
@eric846
Merge pull request #7 from envoyproxy/master
b8c25a5 
merge from upstream
@eric846
initial commit
1c19c68 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix comments
7050686 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix format
0776563 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
rename adaptive_rps to adaptive_load
16fd8f6 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add field_selector in example
c383010 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix example comment
6e1a483 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix format
4ef1140 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add support for fault injection headers
4111bf4 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
replace linear and binary search with exponential search
871a959 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add InputVariableSetter mechanism
1fd77c1 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add input variable setter to build file
edc36b2 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix syntax errors
4d0364e 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
rename samples/adaptive_rps
aed6d94 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
improve comments, change step controller initial value from int64 to …
d9ae87d 
…double

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add proto validation rules, fix comments, make rps the default input_…
a05a6f5 
…variable_setter

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix comment wording
8cd4d21 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
simplify protos, add defaults, specify required or optional
d814a96 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add missing newline
5f5a885 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Kick CI
7e20a78 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
simplify protos
9048267 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix format
306c0ec 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
fix some optional field comments and rules
d33f543 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge pull request #10 from envoyproxy/master
442cca9 
merge from upstream
@eric846
add Nighthawk status field in BenchmarkResult as nested nighthawk.cli…
677b783 
…ent.Output turns out not to include the status

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
switch to standard Envoy plugin config proto, add prefix to internal …
cefb366 
…plugin names, log thresholds only once per session

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into adaptive-rps-protos2
f3684df 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
create headers
5463051 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
eric846 added 17 commits July 25, 2020 22:52
@eric846
remove nested namespace, remove redundant _include in target names
5fc4db4 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
merge from upstream
64e7852 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into adaptive-rps-headers
12807f1 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
merge from upstream
e8e960f 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
6306b4e 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
1ece783 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
70705e9 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
e576bc1 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
1fca528 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
ed32856 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
eecf00d 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
13179fb 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
3eb89e6 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add const to adaptive load helper interface methods, clean up deps an…
070c04d 
…d headers

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
add mocks for adaptive load helpers
1110cf2 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge remote-tracking branch 'upstream/master' into master2
fae60ad 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846
Merge branch 'master2' into adaptive-rps-mocks
12852c6 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846 eric846 marked this pull request as ready for review September 12, 2020 04:01
@eric846 eric846 added the waiting-for-review A PR waiting for a review. label Sep 12, 2020
@eric846
fix typo
80578fa 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
oschaaf
oschaaf previously approved these changes Sep 12, 2020
View reviewed changes
dubious90
dubious90 reviewed Sep 14, 2020
View reviewed changes
Copy link
Contributor

@dubious90 dubious90 left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple places where some comments would be helpful.

Thanks for breaking these out. They're much easier to digest now!

test/mocks/adaptive_load/mock_metrics_evaluator.h
public:
MockMetricsEvaluator();

MOCK_CONST_METHOD3(EvaluateMetric,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the difference between MOCK_CONST_METHOD3 and MOCK_CONST_METHOD1 just the number of arguments to the method being mocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

test/mocks/adaptive_load/mock_metrics_evaluator.h

namespace Nighthawk {

class MockMetricsEvaluator : public MetricsEvaluator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a description to this just clarifying that it's a MetricsEvaluator where all of the overloaded functions... are no-ops, I think? The name obviously goes a long way, but a short comment would help people know how to use it.

Maybe adding a couple lines of example usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Super helpful. Thank you

test/mocks/adaptive_load/mock_session_spec_proto_helper.h

namespace Nighthawk {

class MockAdaptiveLoadSessionSpecProtoHelper : public AdaptiveLoadSessionSpecProtoHelper {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here - just a class-level comment explaining what this does / provide one example usage if that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/mocks/common/mock_nighthawk_service_client.h

namespace Nighthawk {

class MockNighthawkServiceClient : public NighthawkServiceClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dubious90 dubious90 added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 14, 2020
@eric846
add comments to mock classes
50bc113 
Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>
@eric846 eric846 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 14, 2020
dubious90
dubious90 approved these changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@dubious90 dubious90 left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for adding the comments

test/mocks/adaptive_load/mock_metrics_evaluator.h

namespace Nighthawk {

class MockMetricsEvaluator : public MetricsEvaluator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super helpful. Thank you

@dubious90 dubious90 merged commit e744a10 into envoyproxy:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dubious90 dubious90 dubious90 approved these changes

@oschaaf oschaaf oschaaf left review comments

Assignees
No one assigned
Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric846 @dubious90 @oschaaf