-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
access logging: gRPC logger retry to establish underlying stream connection #17469
Conversation
Signed-off-by: Shikugawa <rei@tetrate.io>
lgtm! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/wait
/wait |
…er-retry Signed-off-by: Shikugawa <rei@tetrate.io>
@lizan friendly ping. |
/retest |
Retrying Azure Pipelines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch can you do a non-tetrands review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one Q: how does this behave when we are using the Google gRPC async client vs. the in-built Envoy async gRPC client?
It seems opaque behavior on Google gRPC with this API. Maybe we should configure channel args for retry and buffer on google_grpc configuration. envoy/source/common/grpc/google_async_client_impl.cc Lines 172 to 202 in 3b0a4a0
If we'd like to integrate between Envoy gRPC and Google gRPC for retry and buffer, we should have API migration for envoy's retry policy and Google gRPC's channel args. Is there any plan to work on this? |
I don't think there is a solid story for how to get the retry/buffering semantics aligned across the gRPC implementations. This really touches on the fact that we have not made any progress on unifying the gRPC implementations more broadly. I think for this PR, maybe just clarify in the release notes and comments that this only works with the Envoy gRPC client and we can merge. |
…er-retry Signed-off-by: Shikugawa <rei@tetrate.io>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
@htuch ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@htuch @Shikugawa the merge bring back the generated_api_shadow into main repo. |
@durd07 Thank you for the notification. This is no longer needed and just my mistake. #18911 |
@durd07 what other examples are there? If this is common I can add a hook, otherwise I think we will soon forget about generated_api_shadow/ and this won't sneak back in. |
@htuch , yes, you're right. maybe we will forget about it soon. I met this situation at least twice. but I think it is not necessary to emphasis the alreay not exist codes. please forget it. |
Signed-off-by: Shikugawa rei@tetrate.io
Commit Message: Add a retry mechanism to the grpc access logger. This retry mechanism currently only supports a simple retry count. Also, at the moment, retries are only fired when the gRPC stream fails to be established, and nothing happens if the stream is successfully established once and a reset is issued.
Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes: Required
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]