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.

Sign up for GitHub

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

Jump to bottom

Fix rerun_on description for document #1432

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix rerun_on description for document #1432

wants to merge 1 commit into from

Conversation

y-ken
Copy link
Contributor

@y-ken y-ken commented Jul 7, 2020 •
edited
Loading

follow-up for dbc12e4#diff-828f306a0ac2fd6cd570c28c4b2f1cd2

yoyama 17:54
The document is misleading. The description only focus when attempt already exists.
If no attempt for the session time, require> always kick the child workflow.

context:
https://treasure-data.slack.com/archives/C094GC4JK/p1594110936030000

@y-ken y-ken requested review from yoyama and LeenSun July 7, 2020 11:57
@y-ken y-ken changed the title Fix rerun_on description Fix rerun_on description for document Jul 8, 2020
@y-ken y-ken added the document label Jul 8, 2020
@y-ken
Copy link
Contributor Author

y-ken commented Jul 8, 2020

@LeenSun could you please take a look?

yoyama
yoyama reviewed Oct 29, 2020
View reviewed changes
digdag-docs/src/operators/require.md Outdated
@@ -74,7 +74,7 @@

*rerun_on* control require> really kicks or not if the attempt for the dependent workflow already exists.
* *none* ... Not kick the workflow if the attempt already exists.
* *failed* ... Kick the workflow if the attempt exists and its result is not success.
* *failed* ... Kick the workflow if the attempt not exists or its result was not success.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think "attempt not exists" is proper because this option assumes existence of the attempt as the following description:

rerun_on control require> really kicks or not if the attempt for the dependent workflow already exists.

Copy link
Contributor Author

@y-ken y-ken Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In the actual behavior, rerun_on: failed will generate a new attempt even if the attempt_id at the specific session_time has not been issued. so, how about this description?

*failed* ... Kick the workflow if the attempt may exists and its result is not success.

yoyama
yoyama requested changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@yoyama yoyama left a comment

Choose a reason for hiding this comment

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

As comment, I don't think the fix is not proper.

@y-ken y-ken requested a review from yoyama October 29, 2020 10:19
@y-ken
Copy link
Contributor Author

y-ken commented Oct 29, 2020

@yoyama Updated PR, Would you please take a look?

yoyama
yoyama reviewed Oct 30, 2020
View reviewed changes
digdag-docs/src/operators/require.md Outdated
@@ -74,7 +74,7 @@

*rerun_on* control require> really kicks or not if the attempt for the dependent workflow already exists.
* *none* ... Not kick the workflow if the attempt already exists.
* *failed* ... Kick the workflow if the attempt exists and its result is not success.
* *failed* ... Kick the workflow if the attempt may exists and its result is not success.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I can't understand the meaning of using 'may'. in this description.

BTW, your original concern is rerun_on: failed with the situation when the attempt does not exist.
But rerun_on is only effective if the attempt exists as described.
If it is confused or uncertain, adding description to rerun_on may be better as follows:

*rerun_on* control require> really kicks or not if the attempt for the dependent workflow already exists. 
If the attempt does not exist, the dependent workflow is always kicked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the attempt does not exist, the dependent workflow is always kicked.

sounds great!

@y-ken y-ken requested a review from yoyama October 30, 2020 07:31
@y-ken
Copy link
Contributor Author

y-ken commented Oct 30, 2020

@yoyama I have updated doc as you suggested. 🙆‍♂️

@yoyama
Copy link
Contributor

yoyama commented Dec 24, 2020

As result of large change in master branch, there are many conflicts happened.
Could you resolve it ?

@y-ken y-ken force-pushed the require-rerun_on-doc branch from 037b780 to 1b73277 Compare December 24, 2020 09:27
@y-ken
Copy link
Contributor Author

y-ken commented Dec 24, 2020

@yoyama I have fixed conflicts. please take a look

@szyn szyn force-pushed the master branch 3 times, most recently from c55e9cc to bd66ae8 Compare November 25, 2021 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeenSun LeenSun Awaiting requested review from LeenSun

@yoyama yoyama Awaiting requested review from yoyama

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
document
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@y-ken @yoyama