Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Make wait task async. #3775

Merged
merged 6 commits into from Sep 16, 2023
Merged

Make wait task async. #3775

merged 6 commits into from Sep 16, 2023

Conversation

manan164
Copy link
Contributor

Pull Request type

  • Feature

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Currently, the waiting task can not be less than 30 seconds because the wait task relies on the sweeper to get completed and the sweeper runs every 30 seconds for the workflow.
Now we can decrease the workflowOffsetTimeout to some lower value so that workflow will get evaluated faster.
However, this approach will put a lot of load on the platform since workflows are evaluated faster.
This PR changes the wait task from sync to async mechanism.

Describe the new behavior from this PR, and why it's needed
Issue #
#3403
Alternatives considered

Using workflowOffsetTimeout with a lower value.

@v1r3n v1r3n merged commit 50b6fd4 into Netflix:main Sep 16, 2023
2 checks passed
Date expiryDate = parseDate(until);
long timeInMS = expiryDate.getTime();
long now = System.currentTimeMillis();
long seconds = ((timeInMS - now) / 1000) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

@manan164 Hello, I don't understand why we need to add 1 to the "seconds" variable. It seems to occasionally cause testWaitUntil to fail: https://github.com/Netflix/conductor/actions/runs/6294685428/job/17086871264?pr=3779

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Young-Zen , good point. I think it may be for rounding purposes. I will change this. Thanks for pointing this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Young-Zen , I have raised fix for this, #3786

@manan164 manan164 mentioned this pull request Sep 27, 2023
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants