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

Allow SDK to handle speculative WFT with command events #1509

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

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 Quinn-With-Two-Ns requested a review from a team as a code owner June 11, 2024 23:32
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Makes sense to me, just minor comment changes

internal/internal_task_handlers.go Outdated Show resolved Hide resolved
internal/internal_task_handlers.go Outdated Show resolved Hide resolved
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.

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?

@Quinn-With-Two-Ns
Copy link
Contributor Author

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
Copy link
Member

cretz commented Jun 12, 2024

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
Copy link
Contributor Author

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.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit a7c8208 into temporalio:master Jun 12, 2024
13 checks passed
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.

3 participants