serviceconfig: Return errors instead of skipping invalid retry policy config #7905
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
easwars
approved these changes
Dec 5, 2024
service_config.go
Outdated
| @@ -278,8 +278,7 @@ func convertRetryPolicy(jrp *jsonRetryPolicy, maxAttempts int) (p *internalservi | |||
| jrp.MaxBackoff <= 0 || | |||
| jrp.BackoffMultiplier <= 0 || | |||
| len(jrp.RetryableStatusCodes) == 0 { | |||
| logger.Warningf("grpc: ignoring retry policy %v due to illegal configuration", jrp) | |||
| return nil, nil | |||
| return nil, fmt.Errorf("invalid retry policy (%v): ", jrp) | |||
There was a problem hiding this comment.
I know this is existing code, but this seems to violate the style guide and is clearly hard to read given that the return statement is indented at the same width corresponding to the above conditional. See: go/go-style/decisions#conditional-formatting
Could we just move all the conditions into a single line?
There was a problem hiding this comment.
Also, should we use pretty.JSON or a different formatting directive like %+v or %#v?
service_config_test.go
Outdated
Comment on lines
477
to
488
| scjs: `{ | ||
| "methodConfig": [{ | ||
| "name": [{"service": "foo"}], | ||
| "retryPolicy": { | ||
| "maxAttempts": 2, | ||
| "initialBackoff": "2s", | ||
| "maxBackoff": "10s", | ||
| "backoffMultiplier": 2, | ||
| "retryableStatusCodes": ["UNAVAILABLE"] | ||
| } | ||
| }] | ||
| }`, |
There was a problem hiding this comment.
Can we have this indented like how it is in other subtests.
service_config_test.go
Outdated
| "retryableStatusCodes": ["UNAVAILABLE"] | ||
| } | ||
| }] | ||
| }`, wantErr: true, |
There was a problem hiding this comment.
Nit: Here and elsewhere, can we have wantErr on a separate line so that it is more readily visible.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7905 +/- ##
=======================================
Coverage 82.07% 82.07%
=======================================
Files 377 377
Lines 38179 38180 +1
=======================================
+ Hits 31335 31336 +1
- Misses 5545 5547 +2
+ Partials 1299 1297 -2
|
purnesh42H
pushed a commit
to purnesh42H/grpc-go
that referenced
this pull request
Dec 18, 2024
tpapagian
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
grpc/grpc-go#7905 requires that the number of retres to greater than 1. This is part of google.golang.org/[email protected] and this update makes tests to fail. Signed-off-by: Anastasios Papagiannis <[email protected]>
tpapagian
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
grpc/grpc-go#7905 requires that the number of retres to greater than 1. This is part of google.golang.org/[email protected] and this update makes tests to fail. Signed-off-by: Anastasios Papagiannis <[email protected]>
olsajiri
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
The grpc update to v1.70.0 and especially [1] requires retries to be > 1 in order to have a valid retry policy. As we already do +1 later, let's set the default to 1. [1] grpc/grpc-go#7905 Suggested-by: Anastasios Papagiannis <[email protected]> Signed-off-by: Jiri Olsa <[email protected]>
olsajiri
added a commit
to cilium/tetragon
that referenced
this pull request
Jan 24, 2025
The grpc update to v1.70.0 and especially [1] requires retries to be > 1 in order to have a valid retry policy. As we already do +1 later, let's set the default to 1. [1] grpc/grpc-go#7905 Suggested-by: Anastasios Papagiannis <[email protected]> Signed-off-by: Jiri Olsa <[email protected]>
simonswine
added a commit
to simonswine/pyroscope
that referenced
this pull request
Apr 16, 2025
This change is a consequence by the grpc-go change in v1.70.0, which got merged here: grpc/grpc-go#7905 As we do call `WithDisableRetry()`, it should be a no-op.
simonswine
added a commit
to simonswine/pyroscope
that referenced
this pull request
Apr 16, 2025
This change is a consequence by the grpc-go change in v1.70.0, which got merged here: grpc/grpc-go#7905 As we do call `WithDisableRetry()`, it should be a no-op.
simonswine
added a commit
to simonswine/pyroscope
that referenced
this pull request
Apr 17, 2025
This change is a consequence by the grpc-go change in v1.70.0, which got merged here: grpc/grpc-go#7905 As we do call `WithDisableRetry()`, it should be a no-op.
simonswine
added a commit
to grafana/pyroscope
that referenced
this pull request
Apr 17, 2025
* chore: Update prometheus to version v3. Note this will relax the label validation and allow the newer metric selectors format to allow utf8 symbols. * Migrate testutil.NewTestingLogger * Do usage group comparision by values Instead fully equality of the matcher object, which breaks with the latest prometheus version. * No metric names are incorrect other than empty and non utf-8 * Use correct retry config This change is a consequence by the grpc-go change in v1.70.0, which got merged here: grpc/grpc-go#7905 As we do call `WithDisableRetry()`, it should be a no-op. * Remove method config
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to A21 and A6, an invalid retry policy should result in full service config rejection instead of just skipping the policy. This is a behavior change but also a bug fix.
RELEASE NOTES: