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

[FLINK-32087][checkpoint] Introduce space amplification statistics of file merging #24762

Merged
merged 1 commit into from May 14, 2024
Merged

[FLINK-32087][checkpoint] Introduce space amplification statistics of file merging #24762

merged 1 commit into from May 14, 2024

Conversation

fredia
Copy link
Contributor

@fredia fredia commented May 8, 2024

What is the purpose of the change

This PR introduces space amplification statistics of file merging.

Brief change log

  • Add SpaceStat class in FileMergingSnapshotManager.
  • Update SpaceStat on physical/logical files's creation/deletion

Verifying this change

Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.

This change added tests and can be verified as follows:

  • Add FileMergingSnapshotManagerTestBase#testSpaceStat
  • Update FileMergingSnapshotManagerTestBase#testRestore
  • Update testCreateAndReuseFiles/testCheckpointNotification

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, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented May 8, 2024 •
edited

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@fredia fredia requested a review from Zakelly May 8, 2024 04:59
Zakelly
Zakelly reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

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

Thanks for the pr, i left some comments

...rc/main/java/org/apache/flink/runtime/checkpoint/filemerging/FileMergingSnapshotManager.java Outdated
long physicalFileSize,
long logicalFileCount,
long logicalFileSize) {
this.physicalFileCount = new AtomicLong(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it this.physicalFileCount = new AtomicLong(physicalFileCount); ?

...ain/java/org/apache/flink/runtime/checkpoint/filemerging/FileMergingSnapshotManagerBase.java Outdated
@@ -607,6 +619,7 @@ public void restoreStateHandles(
fileHandle.getScope()))
? physicalFileDeleter
: null;
spaceStat.onPhysicalFileCreate();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid this is not true. If the isManagedByFileMergingManager returns false, there will be no file deleter, so no spaceStat will be updated after file deletion. I'd suggest skip spaceStat if isManagedByFileMergingManager returns false. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree👍, only update spaceStat when isManagedByFileMergingManager return true.

Zakelly
Zakelly reviewed May 13, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Some further comments left.

...ain/java/org/apache/flink/runtime/checkpoint/filemerging/FileMergingSnapshotManagerBase.java Outdated Show resolved Hide resolved
...ain/java/org/apache/flink/runtime/checkpoint/filemerging/FileMergingSnapshotManagerBase.java Outdated
path,
subtaskKey,
fileHandle.getScope()))
managedByFileMergingManager
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that there is owned attribute within the PhysicalFile, how about remove this diversion and wrap everything within the PhysicalFile?

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 moved the setting of FileDeleter into PhysicalFile, and left SpaceStat.update() in FileMergingSnapshotManager

Zakelly
Zakelly reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@Zakelly Zakelly left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Overall LGTM. And will this SpaceStat report to metrics?

flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/filemerging/PhysicalFile.java Outdated Show resolved Hide resolved
@fredia
Copy link
Contributor Author

fredia commented May 14, 2024

Thanks for the update! Overall LGTM. And will this SpaceStat report to metrics?

Currently no, will report to metric in FLINK-32091.

@fredia fredia merged commit 9a5a99b into apache:master May 14, 2024
@fredia fredia deleted the space_amp branch May 16, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zakelly Zakelly Zakelly left review comments

Assignees
No one assigned
Labels
component=Runtime/Checkpointing component=Runtime/StateBackends
Projects
None yet
Milestone
No milestone
3 participants
@fredia @flinkbot @Zakelly