Skip to content

Conversation

mfateev
Copy link
Member

@mfateev mfateev commented Mar 31, 2021

What was changed:

Fixed documentation of activity timeouts and workflow task timeout.

Why?

Documentation was outdated and confusing.

@mfateev mfateev requested a review from flossypurse March 31, 2021 04:39
/**
* Maximum activity execution time after it was sent to a worker. If schedule to close is not
* provided then both this and schedule to start are required.
* Maximum time of a single activity execution attempt.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Maximum time of a single activity execution attempt.
* Maximum time of a single Activity execution attempt.

We want to capitalize these and treat them as proper nouns.

Copy link
Contributor

@flossypurse flossypurse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some suggestions to change sentence formatting and make our core concepts proper nouns.

* provided then both this and schedule to start are required.
* Maximum time of a single activity execution attempt.
*
* <p>Note that Temporal doesn't detect worker process failures directly. It relies on this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>Note that Temporal doesn't detect worker process failures directly. It relies on this
* <p>Note that the Temporal Server doesn't detect Worker process failures directly. It relies on this

* Maximum time of a single activity execution attempt.
*
* <p>Note that Temporal doesn't detect worker process failures directly. It relies on this
* timeout to detect an activity that didn't complete on time. So this timeout should be as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* timeout to detect an activity that didn't complete on time. So this timeout should be as
* timeout to detect that an Activity that didn't complete on time. So this timeout should be as

*
* <p>Note that Temporal doesn't detect worker process failures directly. It relies on this
* timeout to detect an activity that didn't complete on time. So this timeout should be as
* short as the longest possible execution of the activity body. Potentially long running
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* short as the longest possible execution of the activity body. Potentially long running
* short as the longest possible execution of the Activity body. Potentially long running

* <p>Note that Temporal doesn't detect worker process failures directly. It relies on this
* timeout to detect an activity that didn't complete on time. So this timeout should be as
* short as the longest possible execution of the activity body. Potentially long running
* activities must specify HeartbeatTimeout and call {@link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* activities must specify HeartbeatTimeout and call {@link
* Activities must specify HeartbeatTimeout and call {@link

* activities must specify HeartbeatTimeout and call {@link
* ActivityExecutionContext#heartbeat(Object)} periodically for timely failure detection.
*
* <p>If schedule to close is not provided then this timeout is required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>If schedule to close is not provided then this timeout is required.
* <p>If ScheduleToClose is not provided then this timeout is required.


/** Maximum execution time of a single workflow task. Default is 10 seconds. */
/**
* Maximum execution time of a single workflow task. In the majority of cases there is no need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Maximum execution time of a single workflow task. In the majority of cases there is no need
* Maximum execution time of a single Workflow Task. In the majority of cases there is no need

/** Maximum execution time of a single workflow task. Default is 10 seconds. */
/**
* Maximum execution time of a single workflow task. In the majority of cases there is no need
* to change this timeout. Note that this timeout is not related to the overall workflow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* to change this timeout. Note that this timeout is not related to the overall workflow
* to change this timeout. Note that this timeout is not related to the overall Workflow

/**
* Maximum execution time of a single workflow task. In the majority of cases there is no need
* to change this timeout. Note that this timeout is not related to the overall workflow
* duration in any way. It defines for how long workflow can get blocked in case of a workflow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* duration in any way. It defines for how long workflow can get blocked in case of a workflow
* duration in any way. It defines how long the Workflow can get blocked in the case of a Workflow

* Maximum execution time of a single workflow task. In the majority of cases there is no need
* to change this timeout. Note that this timeout is not related to the overall workflow
* duration in any way. It defines for how long workflow can get blocked in case of a workflow
* worker crash.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* worker crash.
* Worker crash.

* duration in any way. It defines for how long workflow can get blocked in case of a workflow
* worker crash.
*
* <p>Default is 10 seconds. Maximum value allowed by the service is 1 minute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>Default is 10 seconds. Maximum value allowed by the service is 1 minute.
* <p>Default is 10 seconds. Maximum value allowed by the Temporal Server is 1 minute.

Copy link
Contributor

@flossypurse flossypurse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added suggestions to new changes -

* task queue (use {@link #setScheduleToStartTimeout(Duration)} to limit it) plus activity
* execution time (use {@link #setStartToCloseTimeout(Duration)} to limit it). Either this
* option or both schedule to start and start to close are required.
* Overall timeout workflow is willing to wait for activity to complete.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Overall timeout workflow is willing to wait for activity to complete.
* Total time that a Workflow is willing to wait for an Activity to complete.

* option or both schedule to start and start to close are required.
* Overall timeout workflow is willing to wait for activity to complete.
*
* <p>ScheduleToCloseTimeout limits total time of the activity execution including retries (use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>ScheduleToCloseTimeout limits total time of the activity execution including retries (use
* <p>ScheduleToCloseTimeout limits the total time of an Activity's execution including retries (use

* Overall timeout workflow is willing to wait for activity to complete.
*
* <p>ScheduleToCloseTimeout limits total time of the activity execution including retries (use
* StartToCloseTimeout to limit a time of a single attempt).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* StartToCloseTimeout to limit a time of a single attempt).
* StartToCloseTimeout to limit the time of a single attempt).

* <p>ScheduleToCloseTimeout limits total time of the activity execution including retries (use
* StartToCloseTimeout to limit a time of a single attempt).
*
* <p>Either this option or StartToClose are required.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>Either this option or StartToClose are required.
* <p>Either this option or StartToClose is required.

/**
* Time activity can stay in task queue before it is picked up by a worker. If schedule to close
* is not provided then both this and start to close are required.
* Time activity can stay in task queue before it is picked up by a worker.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Time activity can stay in task queue before it is picked up by a worker.
* Time that the Activity Task can stay in the Task Queue before it is picked up by a Worker.

* is not provided then both this and start to close are required.
* Time activity can stay in task queue before it is picked up by a worker.
*
* <p>Do not specify this timeout unless using host specific task queues for activity task
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>Do not specify this timeout unless using host specific task queues for activity task
* <p>Do not specify this timeout unless host specific Task Queues for Activity Tasks are being used for routing

* Time activity can stay in task queue before it is picked up by a worker.
*
* <p>Do not specify this timeout unless using host specific task queues for activity task
* routing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove and combine with above comment.

* routing.
*
* <p>ScheduleToStartTimeout is always non-retryable. Retrying after this timeout doesn't make
* sense as it would just put the activity task back into the same task queue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* sense as it would just put the activity task back into the same task queue.
* sense as it would just put the Activity Task back into the same Task Queue.

Copy link
Contributor

@vitarb vitarb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving so we can land right away once Cully's feedback is addressed.

@mfateev mfateev merged commit b642a75 into temporalio:master Apr 6, 2021
@mfateev mfateev deleted the timeout-doc branch April 6, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants