IWF-158: Updating iwf-idl submodule; pinning openapi generator #80
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
iwf/internal_mapper.go
Outdated
| CommandId: t.CommandId, | ||
| FiringUnixTimestampSeconds: t.TimerCommand.FiringUnixTimestampSeconds, | ||
| CommandId: &commandId, | ||
| DurationSeconds: t.TimerCommand.FiringUnixTimestampSeconds, |
There was a problem hiding this comment.
Actually this is different. see indeedeng/iwf-java-sdk#229 and cc @samuel27m who also has the context
There was a problem hiding this comment.
@longquanzheng I added the change to replace FiringUnixTimestampSeconds with DurationSeconds in this MR since it was dependent on the iwf-idl update. Tests all pass now.
…ith new DurationSeconds
| @@ -52,12 +50,12 @@ func NewInternalChannelCommand(commandId, channelName string) Command { | |||
| } | |||
| } | |||
|
|
|||
| func NewTimerCommand(commandId string, firingTime time.Time) Command { | |||
There was a problem hiding this comment.
Similar in Java using Duration, The standard way in Golang is using time.Duration.
Also this is a breaking change here, not a huge problem to me. But we should make it clear in release -- (that's why I thought haivng a sparate PR makes it easier, but it's okay)
There was a problem hiding this comment.
Actually it may be better to keep compabitility for this api. And create a NewTimerCommandByDuration API.
There was a problem hiding this comment.
How would the TimerCommand.DurationSeconds be populated using this firingTime time.Time param?
int64(firingTime.Sub(time.Now()).Seconds())? And then adding a check if negative, set to 0?
There was a problem hiding this comment.
Yes 👍 that’s the idea (same in Java SDK, we used to convert duration to timestamp)
There was a problem hiding this comment.
We probably should panic if setting a negative timestamp
There was a problem hiding this comment.
This could lead to false panics though right? E.g.
firingTime = now + 500ms
... blah ... (this ends up taking 600ms)
NewTimer(firingTime) { now -100ms }
There was a problem hiding this comment.
yeah, good catch.
Probably mark this method as "deprecated" now, in favor of NewTimerCommandByDuration -- It's more useful in practice and people should migrate to it.
There was a problem hiding this comment.
The reasons that I want to panic is that when this happened in the past, it's usally user workflow code issues, but it's a bit hard to figure out why (they used a static/constant for the timerCommand, whcih got the converted value forever during after deployed).
There was a problem hiding this comment.
What you described is possible, but It should be extremely rare that someone want to sleep for milliseconds. The temporal timer is not accurate at millisecond either. But it's possible that a state api call can take a few seconds, but that's also pretty rare.
I am okay to remove the panic, you can decide it.
No description provided.