-
Notifications
You must be signed in to change notification settings - Fork 172
Async package delivery sample #746
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
Conversation
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
antmendoza
left a comment
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 @tsurdilo
Having different paths within the same workflow (rather than child workflows) makes it difficult to understand what is happening to each parcel in the UI. Perhaps activity.summary could help with this
| ``` | ||
|
|
||
| Run sample multiple times to see different scenarios (delivery failure and retry and delivery cancelation) | ||
| There is a 10% chance delivery is going to be canceled and 20% chane it will fail. No newline at end of file |
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.
| There is a 10% chance delivery is going to be canceled and 20% chane it will fail. | |
| There is a 10% chance delivery is going to be canceled and a 20% chance it will fail. |
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 might be helpful as well to have a comment here (in the README.md) or in the code regarding the event history length and CAN
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.
Thank you. Added
| if (!packetDeliveries.get(deliveryId).getDelivered().isCompleted()) { | ||
| logger.info("Sending cancellation for delivery : " + deliveryId + " and reason: " + reason); | ||
| packetDeliveries.get(deliveryId).cancelDelivery(reason); | ||
| } |
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 an else is missing here
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.
its on purpose, we dont want to call cancelDelivery on a packet if its already completed (its completable future completed)
| // more | ||
| while (true) { | ||
| sleep(3); | ||
| // for "fun", reverse the list we get from delivery confirmation list |
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.
this comment is duplicated
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.
Thank you. Removed duplicate
| for (int i = 0; i < 4; i++) { | ||
| try { | ||
| // Perform the heartbeat. Used to notify the workflow that activity execution is alive | ||
| context.heartbeat(i); |
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.
Is the idea here to have a "long-running" activity that can be cancelled? Activity execution latency in this example is between 5-7 ms and heartbeatTimeout is 2 seconds.
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.
its to handle cancelation in activity, similar to HelloCancellation sample
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.
Understand, . Of all the times I’ve run the example, none have resulted in a cancelled activity. maybe 7 ms is not enough, but I will try again later
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.
Yeah you can change the "percentages" of cancel / failure to increase change for cancelation locally
| for (int i = 0; i < 4; i++) { | ||
| try { | ||
| // Perform the heartbeat. Used to notify the workflow that activity execution is alive | ||
| context.heartbeat(i); |
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.
Understand, . Of all the times I’ve run the example, none have resulted in a cancelled activity. maybe 7 ms is not enough, but I will try again later
Signed-off-by: Tihomir Surdilovic <[email protected]>
Signed-off-by: Tihomir Surdilovic <[email protected]>
Packet delivery sample - runs multiple packet deliveries paths async within same execution