[history] refactor history client with timeout wrapper #5728
Conversation
shijiesheng
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted files
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018e14e4-59fe-458c-a46a-cab4fa32ae23Details
💛 - Coveralls |
client/clientfactory.go
Outdated
| @@ -41,6 +41,7 @@ import ( | |||
| "github.com/uber/cadence/client/wrappers/grpc" | |||
| "github.com/uber/cadence/client/wrappers/metered" | |||
| "github.com/uber/cadence/client/wrappers/thrift" | |||
| timoutwrapper "github.com/uber/cadence/client/wrappers/timeout" | |||
| if ctx == nil { | ||
| ctx = context.Background() | ||
| } | ||
| var cancelFunc func() |
There was a problem hiding this comment.
nit: cancelFunc is not used in this scope and can be moved inside if block
There was a problem hiding this comment.
no this is to avoid ctx being scoped.
shijiesheng
force-pushed
the
history-client-timeout
branch
from
70e36dc
to
4bec138
Compare
| op := func(ctx context.Context, peer string) error { | ||
| var err error | ||
| ctx, cancel := c.createContext(ctx) | ||
| defer cancel() | ||
| response, err = c.client.RespondCrossClusterTasksCompleted(ctx, request, append(opts, yarpc.WithShardKey(peer))...) | ||
| return err | ||
| } |
There was a problem hiding this comment.
unless I'm misreading things, this is very different from the now-generated code.
it may be an improvement (createContext is rather terrifying iirc) but it's not clear this is correct.
There was a problem hiding this comment.
I didn't get what is the concern about correctness?
There was a problem hiding this comment.
I think for this method it's correct. What's incorrect is these methods below don't have timeout but now were generated incorrectly with timeout. I'll fix it in the next pr.
GetReplicationMessages
GetDLQReplicationMessages
CountDLQMessages
ReadDLQMessages
PurgeDLQMessages
MergeDLQMessages
GetCrossClusterTasks
GetFailoverInfo
There was a problem hiding this comment.
previously this was potentially modifying the context on each op call during (possibly multiple) request redirection retries.
now it's only doing that once, and it applies across all ops. (well before that, as it's the outer-most layer currently)
that's not necessarily a small change.
There was a problem hiding this comment.
timeout wrapper is still inside the "redirect client". So I think it's the same.
Basically
ctx, cancel := c.createContext(ctx)
defer cancel()
response, err = c.client.RespondCrossClusterTasksCompleted(ctx, request, append(opts, yarpc.WithShardKey(peer))...)
is modified to
response, err = c.wrappedClient.RespondCrossClusterTasksCompleted(ctx, request, append(opts, yarpc.WithShardKey(peer))...)
What changed? #5728 code generated history client should exclude some endpoints. Why? Some replication endpoints don't have timeout before How did you test it? unit test
What changed?
Why?
make history client simple again
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes