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

Expose session state #1024

Merged

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Expose session state to users so they can understand if their activity failed because the session worker failed.

resolves:#1021

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner January 31, 2023 22:45
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.

For this one we might want a test that confirms that session state shows as failed if an in-flight activity fails due to session failure. My concern is something crashes the session

Not sure how such a test looks (an integration test or maybe a "features" test that shuts the session worker down).

SessionStateClosed = internal.SessionStateClosed

// SessionStateFailed means the session worker was detected to be down and the session cannot be used to schedule new activities.
SessionStateFailed = internal.SessionStateFailed
Copy link
Member

Choose a reason for hiding this comment

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

How about the SessionState type itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm yes I guess I should. Originally I didn't think I needed to, but If someone wants to make a function that takes the state as a parameter it would be useful to expose.

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

For this one we might want a test that confirms that session state shows as failed if an in-flight activity fails due to session failure. My concern is something crashes the session

I can make an integration test for this.

I will also note again it is pretty easy, I think, to create a scenario where a session worker fails, but the activity fails firsts so the workflow thinks the session is still open. I think we should document this behaviour.

@cretz
Copy link
Member

cretz commented Feb 1, 2023

I will also note again it is pretty easy, I think, to create a scenario where a session worker fails, but the activity fails firsts so the workflow thinks the session is still open. I think we should document this behaviour.

Hrmm, that a common scenario? I wonder if that makes this whole issue moot. If the user needs to know that their activity was cancelled due to session failure, it seems we can't do that. Sure we can show activity was cancelled due to session complete but not session workflow failure.

Do we have to document that session HeartbeatTimeout has to be less than half that of an activity heartbeat timeout for this to be reported correctly? Or do we just tell users the session may still show open after activity failure due to session worker failure for the duration of the session heartbeat timeout? Or do we just scrap this issue due to these caveats and just document that knowing whether an activity failed due to session failure is unknowable?

@Quinn-With-Two-Ns Quinn-With-Two-Ns force-pushed the session_state branch 2 times, most recently from 6bb53f1 to 1f87a24 Compare February 1, 2023 18:20
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.

Good enough for me, but we need to let that user know that they need to understand what they want to consider a "session failure" because it may be an activity failure on crash before the session reports failure.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit c423fc8 into temporalio:master Feb 1, 2023
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