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-37021][state/forst] Fix incorrect paths when reusing files for checkpoints. #26040

Merged
merged 1 commit into from
Feb 5, 2025
Merged

[FLINK-37021][state/forst] Fix incorrect paths when reusing files for checkpoints. #26040

merged 1 commit into from
Feb 5, 2025

Conversation

AlexYinHan
Copy link
Contributor

@AlexYinHan AlexYinHan commented Jan 21, 2025 •
edited
Loading

What is the purpose of the change

This pull request addresses several bugs in ForSt that may prevent proper file reuse, leading to slower or failed snapshot and restoration operations.

Brief change log

  • Make DataTransferStrategyBuilder function properly.
    • Use URI to tell whether two FileSystem are the same.
    • Use 'Reuse' strategy for Restoration only in CLAIM mode
    • Use 'Reuse' strategy for Snapshot only when SharingFilesStrategy is 'FORWARD_BACKWARD'
  • Ensure we correctly distinguish between dbFilePath and realSourcePath.
  • During Snapshots: do not copy file if it is already owned by JM.

Verifying this change

This change is already covered by existing tests, such as (please describe 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, Kubernetes/Yarn, ZooKeeper: (yes)
  • The S3 file system connector: (no)

Documentation

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

@AlexYinHan
Copy link
Contributor Author

@flinkbot run azure

@flinkbot
Copy link
Collaborator

flinkbot commented Jan 21, 2025 •
edited
Loading

CI report:

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

@Zakelly Zakelly self-requested a review January 22, 2025 04:02
davidradl
davidradl reviewed Jan 22, 2025
View reviewed changes
...-forst/src/main/java/org/apache/flink/state/forst/datatransfer/CopyDataTransferStrategy.java

// Try path-copying first. If failed, fallback to bytes-copying
StreamStateHandle targetStateHandle =
tryPathCopyingToCheckpoint(sourceStateHandle, checkpointStreamFactory, stateScope);
Copy link
Contributor

Choose a reason for hiding this comment

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

tryPathCopyingToCheckpoint can fail with an IOException as well as returning null. It would be better to always return null for a failure I think - otherwise for an IOException we will not try bytesCopyingToCheckpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments. I've modified the code as suggested. PTAL

davidradl
davidradl reviewed Jan 22, 2025
View reviewed changes
...-forst/src/main/java/org/apache/flink/state/forst/datatransfer/CopyDataTransferStrategy.java
filePath.getName());
LOG.trace("Write file to checkpoint: {}, {}", filePath, handleAndLocalPath.getHandle());
return handleAndLocalPath;
return HandleAndLocalPath.of(targetStateHandle, dbFilePath.getName());
}

/**
* Duplicate file to checkpoint storage by calling {@link CheckpointStreamFactory#duplicate} if
* possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: javadoc @params etc are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added params and returns in the javadoc

Zakelly
Zakelly reviewed Jan 22, 2025
View reviewed changes
...rst/src/main/java/org/apache/flink/state/forst/datatransfer/DataTransferStrategyBuilder.java
@Nullable ForStFlinkFileSystem forStFlinkFileSystem,
boolean isDbPathUnderCheckpointPathForSnapshot) {
DataTransferStrategy strategy;
if (forStFlinkFileSystem == null || isDbPathUnderCheckpointPathForSnapshot) {
if (sharingFilesStrategy != SnapshotType.SharingFilesStrategy.FORWARD_BACKWARD
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for ForStIncrementalSnapshotStrategy, the SnapshotType.SharingFilesStrategy.FORWARD is not supported since it breaks the precondition of file sharing between CP and DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I've added a check in ForStIncrementalSnapshotStrategy#asyncSnapshot(). It throws an IllegalArgumentException when encountering SharingFilesStrategy.FORWARD.

@davidradl
Copy link
Contributor

Reviewed by Chi on 23/01/2025 Need a committer to review. Requested minor change has been made.

Zakelly
Zakelly approved these changes Feb 5, 2025
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.

LGTM

fredia
fredia approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@fredia fredia 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, LGTM.

...kend-forst/src/main/java/org/apache/flink/state/forst/fs/filemapping/FileMappingManager.java Show resolved Hide resolved
@Zakelly Zakelly merged commit f975783 into apache:master Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidradl davidradl davidradl left review comments

@Zakelly Zakelly Zakelly approved these changes

@fredia fredia fredia approved these changes

Assignees
No one assigned
Labels
component=<none>
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexYinHan @flinkbot @davidradl @Zakelly @fredia