Skip to content

router: add grpc-retry-on "internal" policy#4644

Merged
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
snowp:internal
Oct 10, 2018
Merged

router: add grpc-retry-on "internal" policy#4644
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
snowp:internal

Conversation

@snowp
Copy link
Contributor

@snowp snowp commented Oct 8, 2018

This allows configuring a gRPC retry policy which retries on internal
(13) responses.

Signed-off-by: Snow Pettersen snowp@squareup.com

Description:
Risk Level: Low
Testing: Added unit test
Docs Changes: Added sub-section to retry-on header
Release Notes: n/a

Snow Pettersen added 2 commits October 8, 2018 14:22
This allows configuring a gRPC retry polic which retries on internal
(13) responses.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
static const uint32_t RETRY_ON_GRPC_DEADLINE_EXCEEDED = 0x40;
static const uint32_t RETRY_ON_GRPC_RESOURCE_EXHAUSTED = 0x80;
static const uint32_t RETRY_ON_GRPC_UNAVAILABLE = 0x100;
static const uint32_t RETRY_ON_GRPC_INTERNAL = 0x101;
Copy link
Member

Choose a reason for hiding this comment

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

0x200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. Obvious in hindsight

Envoy will attempt a retry if the gRPC status code in the response headers is "deadline-exceeded" (4)

internal
Envoy will attempt to retry if the gRPC status code in the response headers is "internal" (13)
Copy link
Member

Choose a reason for hiding this comment

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

nit: empty line after this

Signed-off-by: Snow Pettersen <snowp@squareup.com>
lizan
lizan previously approved these changes Oct 9, 2018
Copy link
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.

LGTM, 2 small things.

deadline-exceeded
Envoy will attempt a retry if the gRPC status code in the response headers is "deadline-exceeded" (4)

internal
Copy link
Member

Choose a reason for hiding this comment

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

Please add a release note for this.

static const uint32_t RETRY_ON_GRPC_DEADLINE_EXCEEDED = 0x40;
static const uint32_t RETRY_ON_GRPC_RESOURCE_EXHAUSTED = 0x80;
static const uint32_t RETRY_ON_GRPC_UNAVAILABLE = 0x100;
static const uint32_t RETRY_ON_GRPC_INTERNAL = 0x200;
Copy link
Member

Choose a reason for hiding this comment

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

This is going to conflict with your other change, so up to you how you want to sequence this.

@mattklein123 mattklein123 self-assigned this Oct 9, 2018
Snow Pettersen added 2 commits October 9, 2018 18:30
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Snow Pettersen added 2 commits October 9, 2018 18:39
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
@mattklein123 mattklein123 merged commit 86355a6 into envoyproxy:master Oct 10, 2018
aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018
This allows configuring a gRPC retry polic which retries on internal
(13) responses.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.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.

3 participants