sqlproxyccl: Add codeUnavailable to the list of error codes#77442
Conversation
|
This change is |
pkg/ccl/sqlproxyccl/connector.go
Outdated
| if err != nil { | ||
| if status.Code(err) != codes.NotFound { | ||
| if status.Code(err) == codes.FailedPrecondition { | ||
| if st, ok := status.FromError(err); ok { |
There was a problem hiding this comment.
If we are unable to retrieve the status, we should still return a codeUnavailable error and break from the loop. I'm worried this case could trigger an infinite loop.
pkg/ccl/sqlproxyccl/connector.go
Outdated
| addr, err := c.Directory.EnsureTenantAddr(ctx, c.TenantID, c.ClusterName) | ||
| if err != nil { | ||
| if status.Code(err) != codes.NotFound { | ||
| if status.Code(err) == codes.FailedPrecondition { |
There was a problem hiding this comment.
nit: it would be nice to flatten this logic. I'm thinking something like
switch {
case err == nil:
return add, nil
case status.Code(err) == codes.FailedPrecondition:
message = "unavailable"
if st, ok := status.FromError(err); ok {
message = st.Message()
}
return "", newErrorf(codeUnavailable, st.Message()
case status.Code(err) != codes.NotFound:
return "", markAsRetriableConnectionError(err)
default:
/* fallback to old resolution rule */
}
14b8cf8 to
514e8da
Compare
alyshanjahani-crl
left a comment
There was a problem hiding this comment.
Do you know how I go about generating errorcode_string.go it says it is generated by "stringer". Since i've added codeUnavailable i need to re-generate this file.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)
pkg/ccl/sqlproxyccl/connector.go, line 323 at r1 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
If we are unable to retrieve the status, we should still return a codeUnavailable error and break from the loop. I'm worried this case could trigger an infinite loop.
Done.
jeffswenson
left a comment
There was a problem hiding this comment.
LGTM
You can use dev to regenerate errcode_string.go
./dev generate
Reviewable status:
8e83ae5 to
897df56
Compare
jaylim-crl
left a comment
There was a problem hiding this comment.
Reviewable status:
897df56 to
56994b2
Compare
Ack, amended the commit msg to remove release note. TFTR |
jaylim-crl
left a comment
There was a problem hiding this comment.
We would sill want Release note: None 😄 Same to PR message I think.
Reviewable status:
Release justification: fixes for high-priority bug in existing functionality Previously, if a tenant cluster had maxPods set to 0 the error returned by directory.EnsureTenantAddr was not treated as a non-retryable error. The tenant directory implementation used in CockroachCloud now identifies this situation and returns a status error with codes.FailedPrecondition and a descriptive message. This patch adds a check for the FailedPrecondition error in connector.lookupAddr. Release note: None
56994b2 to
5bd1ae2
Compare
|
haha, thx for the quick response ;) |
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating backport branch refs/heads/blathers/backport-release-21.2-77442: POST https://api.github.com/repos/cockroachlabs/cockroach/git/refs: 403 Resource not accessible by integration [] Backport to branch 21.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
blathers backport 21.2 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating backport branch refs/heads/blathers/backport-release-21.2-77442: POST https://api.github.com/repos/cockroachlabs/cockroach/git/refs: 403 Resource not accessible by integration [] Backport to branch 21.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Release justification: fixes for high-priority bug in existing functionality
Previously, if a tenant cluster had maxPods set to 0 the error returned by
directory.EnsureTenantAddr was not treated as a non-retryable error.
The tenant directory implementation used in CockroachCloud now identifies
this situation and returns a status error with codes.FailedPrecondition and
a descriptive message.
This patch adds a check for the FailedPrecondition error in
connector.lookupAddr.
Release note: None