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

Fix WAIT task is not working properly (after v3.13.0)#3639

Merged
v1r3n merged 6 commits intoNetflix:mainfrom
yohanyflores:fix-task-not-working-properly
Jun 19, 2023
Merged

Fix WAIT task is not working properly (after v3.13.0)#3639
v1r3n merged 6 commits intoNetflix:mainfrom
yohanyflores:fix-task-not-working-properly

Conversation

@yohanyflores
Copy link
Contributor

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

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

Changes in this PR

Issue #3402

Alternatives considered

? taskModel.getWaitTimeout() + 1
: workflowOffsetTimeout;
if (taskModel.getTaskType().equals(TaskType.TASK_TYPE_WAIT)) {
long deltaInSeconds =
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the wait task has no timeout? This can happen if the WAIT is configured to wait for an external trigger without a timeout value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't take that case into account because it didn't seem to me that the Wait class allowed it, as it requires the use of duration or until. The only way to have a zero timeout is by setting the until parameter to the start of the epoch, January 1, 1970, at 0 hours.

However, I imagine it might be possible in another way that I'm not aware of.

Would you like me to share a code snippet that takes that case into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to update the code. Should I create another Pull Request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also updated the branch in the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yohanyflores , I see that when deltaInSeconds <= 0 shouldn't we mark the postponeDuration to 0 instead of 1 because there is no point in waiting for 1 more second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manan164 Thanks for the change you suggest, you are right!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manan164 I updated the branch with the changes and tests.

@v1r3n v1r3n merged commit 3920aee into Netflix:main Jun 19, 2023
mabelellerker pushed a commit to alfasoftware/conductor that referenced this pull request Aug 4, 2023
…g-properly

Fix WAIT task is not working properly (after v3.13.0)
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.

3 participants