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

api: rename field num_retries to max_retries #11729

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

numerodix
Copy link
Contributor

This PR proposes to rename the RetryPolicy field num_retries to max_retries.

This parameter exists in two places: 1) the RetryPolicy message in the route configuration and 2) the header x-envoy-max-retries. The naming inconsistency is a UX papercut. max_retries feels like right name for what this field is for ie. the maximum number of retries that are permitted.

There is also a stripped down RetryPolicy message which is used by RemoteDataSource which has a num_retries field. I'm including a matching rename of that for consistency.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Martin Matusiak numerodix@gmail.com

Signed-off-by: Martin Matusiak <numerodix@gmail.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #11729 was opened by numerodix.

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.

LGTM, thanks!

@htuch htuch merged commit 286ca92 into envoyproxy:master Jun 24, 2020
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Jun 25, 2020
This PR proposes to rename the RetryPolicy field num_retries to max_retries.

This parameter exists in two places: 1) the RetryPolicy message in the route configuration and 2) the header x-envoy-max-retries. The naming inconsistency is a UX papercut. max_retries feels like right name for what this field is for ie. the maximum number of retries that are permitted.

There is also a stripped down RetryPolicy message which is used by RemoteDataSource which has a num_retries field. I'm including a matching rename of that for consistency.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Martin Matusiak <numerodix@gmail.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
This PR proposes to rename the RetryPolicy field num_retries to max_retries.

This parameter exists in two places: 1) the RetryPolicy message in the route configuration and 2) the header x-envoy-max-retries. The naming inconsistency is a UX papercut. max_retries feels like right name for what this field is for ie. the maximum number of retries that are permitted.

There is also a stripped down RetryPolicy message which is used by RemoteDataSource which has a num_retries field. I'm including a matching rename of that for consistency.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Martin Matusiak <numerodix@gmail.com>
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
This PR proposes to rename the RetryPolicy field num_retries to max_retries.

This parameter exists in two places: 1) the RetryPolicy message in the route configuration and 2) the header x-envoy-max-retries. The naming inconsistency is a UX papercut. max_retries feels like right name for what this field is for ie. the maximum number of retries that are permitted.

There is also a stripped down RetryPolicy message which is used by RemoteDataSource which has a num_retries field. I'm including a matching rename of that for consistency.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Martin Matusiak <numerodix@gmail.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
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.

2 participants