temporalio / sdk-go

Temporal Go SDK
https://docs.temporal.io/application-development?lang=go
MIT License
523 stars 208 forks source link

Allow SDK to handle speculative WFT with command events #1509

Closed Quinn-With-Two-Ns closed 3 months ago

Quinn-With-Two-Ns commented 3 months ago

Allow SDK to handle speculative WFT with command events. Originally I thought this change was going to be quite bad, but I realized I didn't need to touch the cacheing logic and could keep it all in the history iteration.

Quinn-With-Two-Ns commented 3 months ago

I admit not understanding this very much, but is it possible to have an integration test that would break without this change but works now?

No, the server won't trigger this code path yet.

cretz commented 3 months ago

No, the server won't trigger this code path yet.

Arguably this dead code shouldn't make it into the repo until it can be exercised (I fear adding code that we can't confirm works in end-to-end in CI). But if we're confident there are no dangers in adding this and it'll just remain dead code for now and it won't start breaking when server does turn something on, no prob.

Is there a server PR that, when released, will trigger this? Would like us to track the server work to make sure we can add the integration test when we can.

Quinn-With-Two-Ns commented 3 months ago

Arguably this dead code shouldn't make it into the repo until it can be exercised

This code is exercised by unit tests and is called in integration tests as well. There is a clear contract between the SDK and Server we are verifying the SDK side satisfies it's side of the contract.

Would like us to track the server work to make sure we can add the integration test when we can.

Agree, the server work is tracked in Jira, but I'll open up a feature issue for testing this scenario across all SDKs.