Skip to content
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

bulkio: Correctly handle exhausting retries when reading from HTTP. #52281

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

miretskiy
Copy link
Contributor

Fixes #52279

Return an error if we exhaust the retry budget when reading from HTTP.

Release Notes: None

@miretskiy miretskiy requested a review from rohany August 3, 2020 20:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@miretskiy
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed:

@knz
Copy link
Contributor

knz commented Aug 4, 2020

bors r=rohany

craig bot pushed a commit that referenced this pull request Aug 4, 2020
51503: rpc: use tenant client/server certs r=nvanbenschoten a=tbg


With this PR, tenant SQL servers use tenant client certificates to
connect to the tenant server endpoint at the KV layer. They were
previously using the KV-internal node certs.

No validation is performed as of this PR, but this is the obvious
next step.

Follow-up work will assertions that make sure that we don't see
tenants "accidentally" use the node certs for some operations
when they are available (as is typically the case during testing).

Finally, there will be some work on the heartbeats exchanged by
the RPC context. We don't want a SQL tenant's time signal to
ever trigger KV nodes to crash, for example.

Touches #47898.

Release note: None

- cli/flags,config: new flag for tenant KV listen addr
- sql: route tenant KV traffic to tenant KV address
- roachtest: configure --tenant-addr flag in acceptance/multitenant
- rpc: add TenantID to rpc.ContextOptions
- security: slight test improvement
- rpc: pass TenantID to SecurityContext to CertManager
- security: use a single test_certs dir
- security: embed certs for a few hard-coded tenants
- rpc: *almost* use tenant client certs (on tenants)
- rpc: use tenant client/server certs where appropriate


52281: bulkio: Correctly handle exhausting retries when reading from HTTP. r=rohany a=miretskiy

Fixes #52279

Return an error if we exhaust the retry budget when reading from HTTP.

Release Notes: None

Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Yevgeniy Miretskiy <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Build failed (retrying...):

@knz
Copy link
Contributor

knz commented Aug 4, 2020

bors r-
this is a legitimate CI failure:

image

@craig
Copy link
Contributor

craig bot commented Aug 4, 2020

Canceled.

@miretskiy
Copy link
Contributor Author

miretskiy commented Aug 4, 2020 via email

Return an error if we exhaust the retry budget when reading from HTTP.

Release Notes: None
@dt
Copy link
Member

dt commented Aug 13, 2020

bors r=rohany

@miretskiy
Copy link
Contributor Author

oops; didn't realize it didn't merge

@craig
Copy link
Contributor

craig bot commented Aug 13, 2020

Build succeeded:

@craig craig bot merged commit 04be991 into cockroachdb:master Aug 13, 2020
@miretskiy miretskiy deleted the http_retries branch August 13, 2020 14:13
adityamaru added a commit to adityamaru/cockroach that referenced this pull request Aug 26, 2020
PR cockroachdb#52281 fixed a scenario where we would only surface context
cancellation errors, but swallow everything else in the HttpResumer.

This change simply adds a test reinforcing that we do this.

Release note: None

Release justification: non-production code changes
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.

backupccl: panic in backup resumer
5 participants