-
Notifications
You must be signed in to change notification settings - Fork 75
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
Use incremental sequence number as ID for operations between lang and core #173
Conversation
/// EVENT_TYPE_WORKFLOW_TASK_SCHEDULED | ||
/// EVENT_TYPE_WORKFLOW_TASK_STARTED | ||
/// EVENT_TYPE_WORKFLOW_TASK_COMPLETED | ||
/// EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED | ||
fn change_marker_single_timer(marker_type: MarkerType, timer_id: &str) -> TestHistoryBuilder { | ||
fn patch_marker_single_activity(marker_type: MarkerType) -> TestHistoryBuilder { |
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.
Why change this to activity?
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.
Thanks, I'll fix what I can, not sure about exporting WFCommandFut
.
ActivityTaskCompletedEventAttributes { | ||
scheduled_event_id, | ||
started_event_id, | ||
// TODO result: Some(Payloads { payloads: vec![Payload{ metadata: Default::default(), data: vec![] }] }), |
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.
Leftover todos here and below
src/machines/timer_state_machine.rs
Outdated
cancel_timer_fut.await; | ||
Ok(().into()) | ||
}); | ||
|
||
let t = canned_histories::cancel_timer("wait_timer", "cancel_timer"); | ||
let t = canned_histories::cancel_timer("1", "0"); |
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'm not necessarily saying we need to change this, but going from readable names to just numbers harms the readability of these tests some. It would be nice if we could still attach readable IDs to things with sequences somehow, but I understand it's possibly more work than it's worth.
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.
The problem with timers is that they're correlated between the server and core by their timerId
so the reasonable thing to do is use seq
as the timerId
.
6907070
to
474c57d
Compare
474c57d
to
af175d4
Compare
/// We need a field that references the template type T, this works | ||
unused: PhantomData<T>, |
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.
Can start w/ leading underscore - also doc comment not really necessary here, pretty common pattern.
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.
Okay, I'll change
} | ||
} | ||
|
||
impl Future for WFCommandFut { | ||
type Output = UnblockEvent; | ||
impl<T> Unpin for WFCommandFut<T> where T: From<UnblockEvent> {} |
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 believe this should not be necessary, it should get auto-derived
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.
It became necessary, complier was complaining, I think it's the phantom data.
where | ||
T: From<UnblockEvent>, | ||
{ | ||
fn cancel(&self, cx: &WfContext) { |
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.
In principle, the futures could probably store a ref to the WfContext
and then it doesn't need to be passed here, but We don't really have to bother with that right away
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.
let do that in the ... future 😉
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.
Thanks this looks good!
Closes #164
@Sushisource I changed a lot in the prototype rust SDK and tests, I hope you like the new direction.