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

[docs] Clarify restart strategy defaults set by checkpointing #3059

Closed

Conversation

rehevkor5
Copy link
Contributor

  • Added info about checkpointing changing the default restart
    strategy in places where it was missing: the config page and the
    section about the fixed-delay strategy
  • Replaced no-restart with "no restart" so people don't think we're
    referring to a config value
  • Replaced invalid html tag with
  • Fixed bad link to restart strategies page from state.md

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General

    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation

    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build

    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed

- Added info about checkpointing changing the default restart
strategy in places where it was missing: the config page and the
section about the fixed-delay strategy
- Replaced no-restart with "no restart" so people don't think we're
 referring to a config value
- Replaced invalid <it> html tag with <code>
- Fixed bad link to restart strategies page from state.md
@rehevkor5
Copy link
Contributor Author

The changes so far are docs-only, so the CI failure must be unrelated.

- `restart-strategy`: Default [restart strategy]({{site.baseurl}}/dev/restart_strategies.html) to use in case no
restart strategy has been specified for the job.
The options are:
- fixed delay strategy: "fixed-delay".
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use code formatting with backticks instead of " for these as well?

Per default, the no-restart strategy is used.
When checkpointing is activated and no restart strategy has been configured, the job will be restarted infinitely often.
If checkpointing is not enabled, the "no restart" strategy is used.
If checkpointing is activated and the restart strategy has not been configured, the fixed-delay strategy is used with
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we say "If checkpointing is activated and no restart strategy has been explicitly configured, ..."

@uce
Copy link
Contributor

uce commented Jan 9, 2017

Thanks for these clarifications. I'm going to merge it with the next batch. In the current master we also have a new checkpoints page at setup/checkpoints.html where we can add these notes as well.

@asfgit asfgit closed this in 384da0e Jan 9, 2017
asfgit pushed a commit that referenced this pull request Jan 9, 2017
@uce
Copy link
Contributor

uce commented Jan 9, 2017

Thanks again. Just merged this to master (b03ad79) and release-1.2 (2aaa093).

@rehevkor5
Copy link
Contributor Author

@uce Thanks. I didn't get an opportunity to address your comments... is there anything you want me to do?

@rehevkor5
Copy link
Contributor Author

I'm curious as to how you added my commit to master and release-1.2 but it doesn't show up in this PR. The whole "rehevkor5 committed with uce 5 days ago" thing. Github isn't aware that the changes in this PR were merged.

If you have a moment can you describe your workflow is so I can understand what's happening behind the scenes? Is it rebase onto master + merge --ff-only or something? Maybe it's due to the fact that Github is only mirroring another repo? I'd appreciate it!

@uce
Copy link
Contributor

uce commented Jan 9, 2017

Since the comments were very minor I addressed them myself (used the backticks and skipped the change in "If checkpointing is activated..." as it didn't really improve anything).

The work flow was as follows: I checked out your PR branch, rebased it onto master (via git rebase apache/master) and pushed it with another related commit. My related commit had a comment This closes #3059, which tells the ASF bot to close this PR. Then I cherry picked it to the 1.2 branch.

GitHub only notices that the changes were merged for merges to master and if the commit hashes are not changed I think. Since I rebased they did change and I needed the manual close. Does this make sense?

@rehevkor5
Copy link
Contributor Author

It does, thanks! I'll try to remember to include "closes #{pr number}" in my commit messages which might be handy.

@rehevkor5 rehevkor5 deleted the clarify_retry_strategy_defaults branch January 9, 2017 18:32
liuyuzhong7 pushed a commit to liuyuzhong7/flink that referenced this pull request Jan 17, 2017
joseprupi pushed a commit to joseprupi/flink that referenced this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants