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

Test Go and Java SDK client retry behavior #490

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Test Go and Java SDK client retry behavior:

  • Add a gRPC proxy to inject various failure scenarios
  • Renamed some stuff to distinguish this proxy from the http proxy used for some other tests
  • Test can declare they want the proxy, and if so if they want to use it as their default connection
  • Proxy is spun up as part of the harness only if needed and tests would only use it if they ask

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team June 5, 2024 21:00
@Quinn-With-Two-Ns Quinn-With-Two-Ns marked this pull request as ready for review June 10, 2024 18:03
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I plan on leveraging this for #455 where I'll have an HTTP endpoint on the control to fail TLS handshakes. So I may end up needing have the same proxy listen on a TLS-enabled listener too that I can tweak the TLS config on. But I don't think it'll be that hard to plug into this existing one (I hope it's just adding a new listener that's https://pkg.go.dev/crypto/tls#NewListener over the existing one).

Comment on lines +68 to +69
args := make([]string, 0, 64)
args, err := r.config.appendFlags(args)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
args := make([]string, 0, 64)
args, err := r.config.appendFlags(args)
args, err := r.config.appendFlags(nil)

Pedantic, can be lazy if you want, heh

Comment on lines +30 to +32
private static readonly Option<string> summaryUriOption = new(
name: "--summary-uri",
description: "Where to stream the test summary JSONL (not implemented)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we switch this summary thing to HTTP instead of raw TCP one day

Comment on lines +411 to +420
mu sync.Mutex
cv *sync.Cond
gc *grpc.ClientConn
gs *grpc.Server
l net.Listener
wc workflowservice.WorkflowServiceClient
ws workflowservice.WorkflowServiceServer
quitCh chan struct{}
log log.Logger
stateCond *sync.Cond
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have to do anything in this PR, but this is a bit challenging to understand w/ these field names (I understand if came from existing code)

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.

2 participants