Skip to content
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

feat(workflow): Add historySizeInBytes and continueAsNewSuggested to WorkflowInfo (#695) #1223

Merged
merged 14 commits into from
Sep 6, 2023

Conversation

antlai-temporal
Copy link
Contributor

What was changed

Makes visible to workflows the new info fields historySizeInBytes and continueAsNewSuggested

Why?

Makes it easier for workflow code to decide when to continueAsNew() before history becomes too large

Checklist

  1. Closes
    [Feature Request] Add WorkflowInfo.historySizeInBytes #695

  2. How was this tested:
    A new system test and unit test added

  3. Any docs updates needed?

jsdoc of the new fields

@antlai-temporal antlai-temporal requested a review from a team as a code owner August 24, 2023 22:26
@lorensr lorensr changed the title Add historySizeInBytes and continueAsNewSuggested to WorkflowInfo (#695) feat(workflow): Add historySizeInBytes and continueAsNewSuggested to WorkflowInfo (#695) Aug 25, 2023
const MAX_EVENTS = 40_000;
const BATCH_SIZE = 100;
while (workflow.workflowInfo().historyLength < MAX_EVENTS) {
await Promise.all(new Array(BATCH_SIZE).fill(undefined).map((_) => workflow.sleep(1)));
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd use an upsert search attributes instead, it's cheaper to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually pretty fast, and even against the cloud, it will succeed in just 20 round trips (the actual server settings are 4K events, not 40K)

packages/client/src/types.ts Outdated Show resolved Hide resolved
@antlai-temporal
Copy link
Contributor Author

I think the last issue left is to decide whether to rename to historySize(). @cretz has a point that all the others SDKs are using it. I think that having both historyLength() and historySize() in the same interface is confusing, but not a big deal...

@bergundy Shall I go ahead and rename?

@antlai-temporal
Copy link
Contributor Author

@mjameswh and @bergundy Renamed historySizeBytes to historySize, kept attribute in WorkflowExecutionInfo as optional, i.e., replacing zero by undefined, but attribute in WorkflowInfo is no longer optional, i.e.,, defaults to zero, as discussed...

@@ -36,6 +36,15 @@ export interface WorkflowExecutionInfo {
taskQueue: string;
status: { code: proto.temporal.api.enums.v1.WorkflowExecutionStatus; name: WorkflowExecutionStatusName };
historyLength: number;
/**
 * Size of Workflow history in bytes until the current Workflow Task.
Copy link
Contributor

Choose a reason for hiding this comment

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

until the current Workflow Task don't really make sense in this structure. There is no "current workflow task". Similar for This value changes during the lifetime of an Execution. I don't think it adds real value in this context.

Last thing: historySize is available in server versions >= 1.20 (ie. starting with 1.20, not greater than)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment

@mjameswh
Copy link
Contributor

mjameswh commented Sep 5, 2023

@antlai-temporal Just one last minor thing, then I think we can move on.

Copy link
Contributor

@mjameswh mjameswh left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @antlai-temporal!

@antlai-temporal antlai-temporal merged commit aab4149 into temporalio:main Sep 6, 2023
23 checks passed
@antlai-temporal antlai-temporal deleted the add-history-size branch December 8, 2023 02:33
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.

6 participants