Skip to content

dns: enable dns failure refresh rate configuration#8226

Merged
htuch merged 5 commits intoenvoyproxy:masterfrom
venilnoronha:dns-failure-rate
Sep 27, 2019
Merged

dns: enable dns failure refresh rate configuration#8226
htuch merged 5 commits intoenvoyproxy:masterfrom
venilnoronha:dns-failure-rate

Conversation

@venilnoronha
Copy link
Copy Markdown
Member

Description: This adds a new dns_failure_refresh_rate configuration to the Cluster type to allow the configuration of the DNS refresh rate during failures (empty responses).
Risk Level: Low
Testing: Updated tests
Docs Changes: Updated docs
Release Notes: Added an entry describing the change
Fixes #7367

@junr03
Copy link
Copy Markdown
Member

junr03 commented Sep 13, 2019

@snowp do you mind taking a look?

snowp
snowp previously approved these changes Sep 18, 2019
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM

@envoyproxy/api-shepherds for API review

@mattklein123 mattklein123 self-assigned this Sep 19, 2019
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Generally LGTM with some small comments.

/wait

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #8226 was synchronize by venilnoronha.

see: more, trace.

@venilnoronha
Copy link
Copy Markdown
Member Author

The CI is failing for a very weird reason.

envoy/api/v2/cds.proto:703:34: Field number 42 has already been used in "envoy.api.v2.Cluster" by field "dns_failure_refresh_rate".

That's the new field I introduced in this PR, and it thinks the field existed already?

@mattklein123
Copy link
Copy Markdown
Member

You have a merge conflict.

/wait

@mattklein123
Copy link
Copy Markdown
Member

Actually I'm not sure what the issue is. Try merging master.

Copy link
Copy Markdown
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, looks good but a few cleanups requested.

@venilnoronha
Copy link
Copy Markdown
Member Author

@htuch I've addressed your comments in f7ab87b.

@mattklein123 mattklein123 assigned htuch and unassigned mattklein123 Sep 23, 2019
Copy link
Copy Markdown
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, final few comments.
/wait

@venilnoronha
Copy link
Copy Markdown
Member Author

@htuch I've addressed your latest comments in adc0da2.

@venilnoronha venilnoronha force-pushed the dns-failure-rate branch 2 times, most recently from 81979d7 to eb95685 Compare September 25, 2019 01:45
@venilnoronha
Copy link
Copy Markdown
Member Author

unrelated flakes?

@lizan lizan added the api-review-required API review required by @envoyproxy/api-shepherds label Sep 25, 2019
@lizan
Copy link
Copy Markdown
Member

lizan commented Sep 25, 2019

@venilnoronha No, you'll need to update stats_integration_test for the per cluster memory test when you add a new field to cluster.

Copy link
Copy Markdown
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 CI failures.

This adds a new `dns_failure_refresh_rate` configuration to the
`Cluster` type to allow the configuration of the DNS refresh rate during
failures (empty responses).

Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Copy link
Copy Markdown
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!

@htuch htuch merged commit 153bf82 into envoyproxy:master Sep 27, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
This adds a new dns_failure_refresh_rate configuration to the Cluster type to allow the configuration of the DNS refresh rate during failures (empty responses).

Risk Level: Low
Testing: Updated tests
Docs Changes: Updated docs
Release Notes: Added an entry describing the change

Fixes envoyproxy#7367

Signed-off-by: Venil Noronha <veniln@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review-required API review required by @envoyproxy/api-shepherds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add dns_failure_refresh_rate similar to Cluster.dns_refresh_rate

6 participants