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

config: Remove entries from initial resource versions map #6320

Merged
merged 14 commits into from
Mar 25, 2019

Conversation

fredlas
Copy link
Contributor

@fredlas fredlas commented Mar 19, 2019

Description: Remove entry from the "initial resource versions" map when the server informs us that the corresponding resource has gone away.
Risk Level: low
#4991

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas
Copy link
Contributor Author

fredlas commented Mar 19, 2019

A test of what I fixed here wouldn't fit naturally into the SubscriptionTestHarness setup. Is it worth adding a unit test file specific to delta_subscription_impl.h?

@jmarantz
Copy link
Contributor

Going to assign this to someone familiar with this code, but IMO it is worth adding a new unit test file specific to the impl.

@jmarantz
Copy link
Contributor

@dmitri-d can you take a first pass?

@dmitri-d
Copy link
Contributor

lgtm

dmitri-d
dmitri-d previously approved these changes Mar 20, 2019
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; should we have a test for this?

/wait

source/common/config/delta_subscription_impl.h Outdated Show resolved Hide resolved
source/common/config/delta_subscription_impl.h Outdated Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas
Copy link
Contributor Author

fredlas commented Mar 21, 2019

Test added.

Signed-off-by: Fred Douglas <fredlas@google.com>
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.

Thanks, LGTM modulo remaining minor test related comments.

/wait

source/common/config/delta_subscription_impl.h Outdated Show resolved Hide resolved
test/common/config/delta_subscription_impl_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
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, one question...

/wait

source/common/config/delta_subscription_impl.h Outdated Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
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 78ad883 into envoyproxy:master Mar 25, 2019
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.

4 participants