uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
344 stars 130 forks source link

Stop retrying get-workflow-history with an impossibly-short timeout #1171

Closed Groxx closed 2 years ago

Groxx commented 2 years ago

Previously, when a GetWorkflow(...).Get(...) call timed out while waiting for the workflow to complete, this happened:

Which is pretty clearly sub-optimal. Both because we sent a request that is almost guaranteed to fail, and because the error returned to the caller is fairly generic looking / doesn't describe what happened. It just looks like they sent us a bad request / their code is not reasonable somehow.

What happened is that we ran out of time. So this now returns a DeadlineExceeded error, like any other timeout, and does not cause that final request to occur.


These "impossibly short deadline" errors are still entirely possible, e.g. if we are given a context with only 25ms remaining, or 25ms longer than the default poll timeout.

It might be reasonable to eagerly return a timeout in these cases as well... but we don't currently have a way to know what the server's minimum time is. That might be fixable in the future though.

Groxx commented 2 years ago

I should add a test or three for this, and I think I might need a different error message (since I believe it's not guaranteed that it's due to waiting on the workflow, the database could've just failed to respond)... but this works well locally at least.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 0181659a-c6f5-42c6-bd6b-0b78f8c7d72a


Files with Coverage Reduction New Missed Lines %
internal/internal_task_pollers.go 2 81.43%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 01815bd7-df81-4af6-8702-318ee71d3223: 0.01%
Covered Lines: 12395
Relevant Lines: 19439

💛 - Coveralls