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 Controller headers #417

Merged
merged 37 commits into from
Jul 31, 2020
Merged

Adaptive Load Controller headers #417

merged 37 commits into from
Jul 31, 2020

Conversation

eric846
Copy link
Contributor

@eric846 eric846 commented Jul 22, 2020 •
edited
Loading

This is the public C++ API of the Adaptive Load Controller library:

  • A single function: PerformAdaptiveLoadSession()
  • Interfaces for user-defined plugins:
    • MetricsPlugin
    • StepController
    • ScoringFunction
    • InputVariableSetter

After submitting this PR, I will try to send 4 small independent plugin implementation PRs in parallel, then one larger PR with the main driver that depends on all of them, then a PR with the CLI wrapper.

#416

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 <[email protected]>
@eric846
fix comments
7050686 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
0776563 
Signed-off-by: eric846 <[email protected]>
@eric846
rename adaptive_rps to adaptive_load
16fd8f6 
Signed-off-by: eric846 <[email protected]>
@eric846
add field_selector in example
c383010 
Signed-off-by: eric846 <[email protected]>
@eric846
fix example comment
6e1a483 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
4ef1140 
Signed-off-by: eric846 <[email protected]>
@eric846
add support for fault injection headers
4111bf4 
Signed-off-by: eric846 <[email protected]>
@eric846
replace linear and binary search with exponential search
871a959 
Signed-off-by: eric846 <[email protected]>
@eric846
add InputVariableSetter mechanism
1fd77c1 
Signed-off-by: eric846 <[email protected]>
@eric846
add input variable setter to build file
edc36b2 
Signed-off-by: eric846 <[email protected]>
@eric846
fix syntax errors
4d0364e 
Signed-off-by: eric846 <[email protected]>
@eric846
rename samples/adaptive_rps
aed6d94 
Signed-off-by: eric846 <[email protected]>
@eric846
improve comments, change step controller initial value from int64 to …
d9ae87d 
…double

Signed-off-by: eric846 <[email protected]>
@eric846
add proto validation rules, fix comments, make rps the default input_…
a05a6f5 
…variable_setter

Signed-off-by: eric846 <[email protected]>
@eric846
fix comment wording
8cd4d21 
Signed-off-by: eric846 <[email protected]>
@eric846
simplify protos, add defaults, specify required or optional
d814a96 
Signed-off-by: eric846 <[email protected]>
@eric846
add missing newline
5f5a885 
Signed-off-by: eric846 <[email protected]>
@eric846
Kick CI
7e20a78 
Signed-off-by: eric846 <[email protected]>
@eric846
simplify protos
9048267 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
306c0ec 
Signed-off-by: eric846 <[email protected]>
@eric846
fix some optional field comments and rules
d33f543 
Signed-off-by: eric846 <[email protected]>
@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 <[email protected]>
@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 <[email protected]>
@eric846
Merge remote-tracking branch 'upstream/master' into adaptive-rps-protos2
f3684df 
Signed-off-by: eric846 <[email protected]>
@eric846
create headers
5463051 
Signed-off-by: eric846 <[email protected]>
@mum4k mum4k added the waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. label Jul 23, 2020
@eric846
split build target, get rid of ostream, change InputValueSetter to us…
b9c8f2b 
…e a reference, add file comments

Signed-off-by: eric846 <[email protected]>
@eric846
Copy link
Contributor Author

eric846 commented Jul 24, 2020

/retest

@repokitteh-read-only
Copy link

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #417 (comment) was created by @eric846.

see: more, trace.

@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 Jul 24, 2020
mum4k
mum4k requested changes Jul 24, 2020
View reviewed changes
Copy link
Collaborator

@mum4k mum4k left a comment

Choose a reason for hiding this comment

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

Complete review now, nothing major just two open discussions.

include/nighthawk/adaptive_load/BUILD Outdated
envoy_package()

envoy_basic_cc_library(
name = "adaptive_load_controller_include",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious - why are we adding the "_include" suffix which is common for all of these? Since it is the same in all the targets, it probably doesn't add any value. Why not just call this "adaptive_load_controller"? It also makes it easier to figure out what to include when the name of the target matches the name of the library.

Applies to all the targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed "_include" from all targets.

include/nighthawk/adaptive_load/adaptive_load_controller.h Outdated
#include "api/client/service.grpc.pb.h"

namespace Nighthawk {
namespace AdaptiveLoad {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you Otto, can you help me understand the reasoning behind it being more convenient? One could argue the exact opposite, since once we introduce the sub-namespace we will have to carry it in all headers when using symbols from the namespace.

We can indeed introduce it, but it would be good to understand what problem we are solving by using a language feature, rather than just using it to match a directory structure. Please feel free to let me know if I misunderstood the comment.

@mum4k mum4k 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 Jul 24, 2020
@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 Jul 26, 2020
mum4k
mum4k requested changes Jul 29, 2020
View reviewed changes
Copy link
Collaborator

@mum4k mum4k 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, please merge master and this will be ready for merging.

@mum4k mum4k 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 Jul 29, 2020
@eric846
merge from upstream
64e7852 
Signed-off-by: eric846 <[email protected]>
@eric846
Copy link
Contributor Author

eric846 commented Jul 29, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: benchmark (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: test_gcc (failed build)
🔨 rebuilding ci/circleci: test (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #417 (comment) was created by @eric846.

see: more, trace.

@eric846
Copy link
Contributor Author

eric846 commented Jul 29, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: test_gcc (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: benchmark (failed build)
🔨 rebuilding ci/circleci: test (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #417 (comment) was created by @eric846.

see: more, trace.

@eric846
Copy link
Contributor Author

eric846 commented Jul 29, 2020

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #417 (comment) was created by @eric846.

see: more, trace.

@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 Jul 29, 2020
@eric846 eric846 requested a review from mum4k July 29, 2020 21:14
@mum4k
Copy link
Collaborator

mum4k commented Jul 31, 2020

@oschaaf status shows you may be still requesting changes here. Can you please indicate what changes or approve if all looks good?

@mum4k mum4k merged commit 5f4f862 into envoyproxy:master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqustc qqustc qqustc left review comments

@oschaaf oschaaf oschaaf approved these changes

@mum4k mum4k mum4k approved these changes

Assignees

@mum4k mum4k

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.
4 participants
@eric846 @mum4k @qqustc @oschaaf