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.

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

eds: optionalize counting of unknown fields #10982

Merged
merged 13 commits into from
May 12, 2020
Merged

Conversation

pgenera
Copy link
Contributor

@pgenera pgenera commented Apr 28, 2020

Commit Message: In order to speed up eds, don't necessarily visit every proto field to count its validity as WarningValidationVisitor does. This yields a ~30% speed improvement in processing very large updates in EDS.
Risk Level: medium, new feature behind a command line flag.
Testing: Unit and bechmark tests.
Docs Changes: These are probably wrong, thus the draft-ness.
Release Notes: EDS can now ignore unknown dynamic fields, for a ~30% improvement in update processing time. Behind --ignore-unknown-dynamic-fields

jmarantz and others added 2 commits April 28, 2020 12:01
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10982 was opened by pgenera.

see: more, trace.

@jmarantz jmarantz self-assigned this Apr 28, 2020
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

This looks like a great improvement, thanks. Some initial comments..
/wait

docs/root/operations/admin.rst Outdated Show resolved Hide resolved
source/common/upstream/eds.cc Outdated Show resolved Hide resolved
source/common/upstream/upstream_impl.cc Outdated Show resolved Hide resolved
test/common/upstream/eds_speed_test.cc Outdated Show resolved Hide resolved
test/common/upstream/eds_speed_test.cc Outdated Show resolved Hide resolved
@htuch htuch self-assigned this Apr 28, 2020
Signed-off-by: Phil Genera <[email protected]>
@pgenera pgenera marked this pull request as ready for review April 30, 2020 17:21
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.
/wait

include/envoy/server/options.h Outdated Show resolved Hide resolved
include/envoy/protobuf/message_validator.h Outdated Show resolved Hide resolved
source/common/upstream/upstream_impl.cc Outdated Show resolved Hide resolved
source/server/options_impl.cc Outdated Show resolved Hide resolved
test/common/upstream/eds_test.cc Show resolved Hide resolved
test/common/upstream/eds_test.cc Outdated Show resolved Hide resolved
@repokitteh-read-only
Copy link

CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #10982 was synchronize by pgenera.

see: more, trace.

Copy link
Member

@htuch htuch 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!
/wait

test/common/upstream/eds_test.cc Outdated Show resolved Hide resolved

bool skipValidation() override { return skip_validation_; }

void setSkipValidation(bool s) { skip_validation_ = s; }
Copy link
Member

Choose a reason for hiding this comment

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

For mocks, we generally don't have setters and expose the fiels, but this doesn't hurt I guess.

test/integration/integration.h Outdated Show resolved Hide resolved
return fmt::format(
"{}_{}",
"{}_{}_{}",
Copy link
Member

Choose a reason for hiding this comment

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

How long does this test take to run now? Generally wary of uncontrolled combinatorial explosision. If it's not too long it's fine, but prefer to have only a meaningful subset of tests validated with this setting.

Copy link
Contributor Author

@pgenera pgenera May 7, 2020

Choose a reason for hiding this comment

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

8.5s (locally on my corp machine), compared to 5s without the change. It's in the middle of the pack of //test/integration:all, with the longest ones in the 60s range.

It's around 20s with RBE, but still pretty average for the integration tests.

test/common/upstream/eds_speed_test.cc Show resolved Hide resolved
docs/root/operations/cli.rst Outdated Show resolved Hide resolved
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo final comment snafu.
/wait

htuch
htuch previously approved these changes May 8, 2020
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Signed-off-by: Phil Genera <[email protected]>
pgenera added 4 commits May 11, 2020 10:40
Signed-off-by: Phil Genera <[email protected]>
Signed-off-by: Phil Genera <[email protected]>
Signed-off-by: Phil Genera <[email protected]>
@pgenera
Copy link
Contributor Author

pgenera commented May 12, 2020

The remaining clang-tidy complaint is pre-existing, I believe.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 703f2fb into envoyproxy:master May 12, 2020
@pgenera pgenera deleted the eds-speed branch May 12, 2020 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants