-
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
kv: gRPC Unavailable errors are ambiguous #17701
Conversation
@@ -73,3 +73,15 @@ variable "controller_root_disk_size" { | |||
variable "controller_root_disk_type" { | |||
default = "pd-standard" # can set this to 'pd-ssd' for persistent SSD | |||
} | |||
|
|||
# Local path to the cockroach binary. An empty value downloads a | |||
# pre-built binary using cockroach_sha. |
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.
This comment doesn't seem quite right.
} | ||
|
||
# SHA of the cockroach binary to download if cockroach_binary is | ||
# unset. Defaults to the latest master build. |
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.
Ditto here.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3. build/jepsen/terraform/main.tf, line 184 at r2 (raw file):
why not curl it directly to the destination? i'm confused about what goes on in this area of the code. pkg/kv/dist_sender.go, line 1177 at r3 (raw file):
Did you mean to remove this comment? Comments from Reviewable |
9c6af2c
to
0c35505
Compare
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. build/jepsen/terraform/main.tf, line 184 at r2 (raw file): Previously, tamird (Tamir Duberstein) wrote…
The destination path is mentioned twice on the preceding line, as well as in the file provisioner above, so it's easier to move/copy the file here than to adjust all of those. All of this is because jepsen expects as input a tarball containing an executable at build/jepsen/terraform/variables.tf, line 78 at r3 (raw file): Previously, benesch (Nikhil Benesch) wrote…
What's wrong with it? pkg/kv/dist_sender.go, line 1177 at r3 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. Comments from Reviewable |
Review status: 3 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. build/jepsen/terraform/variables.tf, line 78 at r3 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Ah, never mind, I misread. I wish terraform had conditions. Comments from Reviewable |
Reviewed 1 of 1 files at r4. build/jepsen/terraform/main.tf, line 146 at r4 (raw file):
was this intentional? none of the commit messages mention it. Comments from Reviewable |
0c35505
to
e3884a4
Compare
Review status: 2 of 4 files reviewed at latest revision, 3 unresolved discussions. build/jepsen/terraform/main.tf, line 146 at r4 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Oops, fixed (the Comments from Reviewable |
Reviewed 2 of 2 files at r5. Comments from Reviewable |
Copied from the pre-cockroachdb#17457 version of the other terraform configs
This error code is used for fail-fast errors (which can be retried unambiguously), but it is also used in other cases (such as a server draining) in which we cannot assume that the previous attempt was not completed. (It's unclear whether this assumption was once true and changed or if it's always been incorrect. The specific source of ambiguous Unavailable errors we're seeing is grpc/grpc-go#1147) This is expected to increase prevalence of AmbiguousResultErrors; this will be fixed in a follow-up change. Fixes cockroachdb#17491
e3884a4
to
bd9e2a6
Compare
Still waiting on an LGTM. |
Reviewed 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8. Comments from Reviewable |
This error code is used for fail-fast errors (which can be retried
unambiguously), but it is also used in other cases (such as a server
draining) in which we cannot assume that the previous attempt was not
completed. (It's unclear whether this assumption was once true and
changed or if it's always been incorrect. The specific source of
ambiguous Unavailable errors we're seeing is grpc/grpc-go#1147)
This is expected to increase prevalence of AmbiguousResultErrors; this
will be fixed in a follow-up change.
Fixes #17491