-
Notifications
You must be signed in to change notification settings - Fork 3.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
release-21.2: sqlproxyccl: Add codeUnavailable to the list of error codes #77848
release-21.2: sqlproxyccl: Add codeUnavailable to the list of error codes #77848
Conversation
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
@jaylim-crl , @jeffswenson since the connector refactor isn't backported, i had to manually make this backport. Pls take another look. |
db98361
to
0bed3a4
Compare
@@ -58,6 +58,10 @@ const backendError = "Backend error!" | |||
// the test directory server. | |||
const notFoundTenantID = 99 | |||
|
|||
// unavailableTenantID is used to trigger a FailedPrecondition error when it is requested in | |||
// the test directory server. | |||
const unavailableTenantID = 99 |
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.
If the tenant IDs are the same, wouldn’t we hit the NotFound case? 🤔
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.
yep haha was WIP, i make changes locally then push to remote branch and pull/test on gceworker.
Should be good now
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 when proxyHandler.Handle calls proxyHandler.outgoingAddress. Release note: None
0bed3a4
to
c9fe7ec
Compare
I tried retriggering the Bazel CI TC build but it still failed.. This is what im seeing as an error
Gonna retry again, and post to dev-inf if i get the same issue. |
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.
once tests pass. I've never seen the CI issue before. As you suggested, dev-inf would be a good place to reach out.
Reviewed 4 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @alyshanjahani-crl)
Dev-inf told me i can ignore this, saying bazel CI doesn't matter for release-21.2 and prior. DIdn't realize that the failure wasn't stopping us from merging. Tests are passing, going to merge. TFTR @jaylim-crl ! |
Backport 1/1 commits from #77442.
/cc @cockroachdb/release
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