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

[FLINK-8657][doc]Fix incorrect description in the document for externalized checkpoint… #5490

Conversation

Projects
None yet
4 participants
@sihuazhou
Copy link
Contributor

sihuazhou commented Feb 14, 2018

What is the purpose of the change

This PR addressed issue FLINK-8657. I checked that external checkpoint also supported rescale both in code and practice. But in the doc it still note that "do not support Flink specific features like rescaling."

Brief change log

  • Fix incorrect description for externalized checkpoint vs savepoint.
  • Fix incorrect description for memory state backend.

Verifying this change

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
@sihuazhou

This comment has been minimized.

Copy link
Contributor Author

sihuazhou commented Feb 14, 2018

@aljoscha Cloud you please have a look at this? I'm afraid I don't understand things correctly.

@sihuazhou sihuazhou changed the title Fix incorrect description in the document for externalized checkpoint… [FLINK-8657][doc]Fix incorrect description in the document for externalized checkpoint… Feb 14, 2018

@aljoscha

This comment has been minimized.

Copy link
Contributor

aljoscha commented Feb 15, 2018

@StefanRRichter Could you have a look at this?

@StefanRRichter

This comment has been minimized.

Copy link
Contributor

StefanRRichter commented Feb 15, 2018

The docs are not "incorrect" and this is on purpose. Background is that, in general, externalized checkpoints are only required to support a subset of the features that savepoints provide and we do not want that user rely on their checkpoints to be rescalable. While this might be true now, this might change for all or some implementations in the future, so we simply do not want to give a guarantee that we then must maintain. For externalized checkpoints, rescaling operations can also come with "surprising" performance impacts for the user.

@sihuazhou

This comment has been minimized.

Copy link
Contributor Author

sihuazhou commented Feb 16, 2018

Aha, I got it. But I also found the description of Memory state backend is wrong in these PR, the description is The MemoryStateBackend can be configured to use asynchronous snapshots. While we strongly encourage the use of asynchronous snapshots to avoid blocking pipelines, please note that this is a new feature and currently not enabled by default., but I found asynchronous snapshots is enabled by default, it seems like out of date, could you please also have a look ? @StefanRRichter

@StefanRRichter

This comment has been minimized.

Copy link
Contributor

StefanRRichter commented Feb 16, 2018

Yes, that is outdated.

@sihuazhou

This comment has been minimized.

Copy link
Contributor Author

sihuazhou commented Feb 17, 2018

@StefanRRichter If that is outdated, could you please merge this PR to fix it?

@StefanRRichter

This comment has been minimized.

Copy link
Contributor

StefanRRichter commented Feb 19, 2018

Thanks for the correction, LGTM 👍

@asfgit asfgit closed this in 5909b5b Feb 19, 2018

@sihuazhou sihuazhou deleted the sihuazhou:fix_doc_bug_for_externalized_checkpoint branch Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.