Repository navigation
Add Nexus link converter for Link_Callback - #322
Conversation
|
|
Evanthx
left a comment
Evanthx
left a comment
There was a problem hiding this comment.
Two nits, but overall I liked it!
| } | ||
| var err error | ||
| if nexusLink, err = ConvertLinkCallbackToNexusLink(v.Callback); err != nil { | ||
| return nil, false |
There was a problem hiding this comment.
This error is lost. Should log it at least, not sure altering this method to return the error is worthwhile!
There was a problem hiding this comment.
I don't feel strongly about it, but I kinda disagree.
The (*nexuspb.Link, bool) return value communicates that the function will return false if there is a problem. So I don't know if us seeing a bad input is enough of a concern to write it into the logs. Kinda like in other languages you might have a func TryParse(s string) (int, bool) function that would swallow errors.
Also, wiring in the context needed to find the right log stream might be pain. And/or writing to STDOUT/STDERR might cause other headaches.
There was a problem hiding this comment.
+1 to what Chris said, I don't think we should log here.
| if err != nil { | ||
| return decodedLink{}, err | ||
| } | ||
| return decodedLink{namespace: segments[0], id: segments[1], runID: segments[2], query: link.URL.Query()}, nil |
There was a problem hiding this comment.
Might be worth checking that there are three elements in segments - or at least setting up a unit test that runs this with an empty segment array and just makes sure it all works nicely or fails nicely!
There was a problem hiding this comment.
Added a check for the number of segments in decodeSegments.
chrsmith
left a comment
chrsmith
left a comment
There was a problem hiding this comment.
Modulo nitpicks this LGTM.
|
|
||
| // ConvertNexusLinkToLinkCallback converts a Nexus Link to Link_Callback. | ||
| // | ||
| // NOTE: Experimental |
There was a problem hiding this comment.
Nit: I'm personally conflicted about this // NOTE: Experimental comment.
On one hand, this code is specifically to support a feature that isn't in a pre-release stage. And we literally call that out as "Experimental". (https://docs.temporal.io/evaluate/product-release-stages)
But I also think that we'd ever plan on removing this code if we didn't ship Worker Callbacks. Or if we did, going back and removing the // NOTE: Experimental comments.
I don't have a good answer for what we should do instead. Just that I personally don't like seeing // NOTE: Experimental in the codebase for files that haven't been touched in years 😅 .
There was a problem hiding this comment.
Yeah, I don't have a good answer either. But I think it's okay to mark it as experimental for now 🤷♀️
What changed?
Added conversion between
commonpb.Link_Callbackand Nexus links totemporalnexus. It uses the same URL format the server introduced in temporalio/temporal#12070.Why?
The server now sends Callback links, but
api-go, and so the Go SDK, doesn't recognize them and drops them. This change lets those links round-trip the same way they do on the server.How did you test it?
Added unit tests.
Potential risks
None.