-
Notifications
You must be signed in to change notification settings - Fork 204
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
Bump SDK to 1.31 #374
Bump SDK to 1.31 #374
Conversation
e2d7fce
to
13dd4fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from #371
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I copied the code verbatim but it fails. would you know why, @yuandrew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, looking, it's not immediately clear why this would suddenly fail, I tested this locally when making the Go SDK change, and it should be a pretty basic UX change, shouldn't be any change to functionality.
Looking at the logs, only 1 activity is completing, instead of the 500 the test expects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, found the issue, this was due to temporalio/sdk-go#1695 changing test behavior to more closely match how server handles duplicate update IDs. Put the suggested fix in a separate comment.
Thanks for catching this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuandrew thank you so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dandavison I revamped this based on your notes and the Java sample.
early-return/starter/main.go
Outdated
"WorkflowID:", updateHandle.WorkflowID(), | ||
"RunID:", updateHandle.RunID()) | ||
|
||
var txResult earlyreturn.Transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should name this something like earlyReturnResult
, or transactionConfirmation
, or something else along those lines. The key point we want to get across is that the transaction hasn't completed yet, but txResult
suggests otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. I can rename it 👍 The java-sample has the same issue.
What was changed