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-31640][network] Write the accumulated buffers to the right storage tier #22652

Merged
merged 2 commits into from
Jun 5, 2023
Merged

[FLINK-31640][network] Write the accumulated buffers to the right storage tier #22652

merged 2 commits into from
Jun 5, 2023

Conversation

TanYuxin-tyx
Copy link
Contributor

@TanYuxin-tyx TanYuxin-tyx commented May 25, 2023 •
edited

What is the purpose of the change

  1. Write the accumulated buffers to the right storage tier

Brief change log

    • Firstly choose the storage tier, then write the accumulated buffers to the right storage tier.*

Verifying this change

  • Added the test of TieredStorageProducerClientTest to verify the logic of choosing storage tiers.

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 documented)

@flinkbot
Copy link
Collaborator

flinkbot commented May 25, 2023 •
edited

CI report:

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

@TanYuxin-tyx TanYuxin-tyx force-pushed the FLINK-31640 branch 3 times, most recently from 2ac75ff to 02532cb Compare May 26, 2023 02:46
xintongsong
xintongsong reviewed May 28, 2023
View reviewed changes
Copy link
Contributor

@xintongsong xintongsong 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 opening the PR, @TanYuxin-tyx. I have left some comments, PTAL. In addition to the in-line comments, I'm a bit confused by the positioning of SubpartitionSegmentIdTracker. Based on the implementation so far, I don't see how it is expected to be used. Does it belong to a specific tier? Or should it be reused by multiple tiers? I wonder if we should postpone the introducing of it to when we introduce the implementation of a specific tier.

...e/flink/runtime/io/network/partition/hybrid/tiered/storage/SubpartitionSegmentIdTracker.java Outdated Show resolved Hide resolved
...e/flink/runtime/io/network/partition/hybrid/tiered/storage/SubpartitionSegmentIdTracker.java Outdated Show resolved Hide resolved
...e/flink/runtime/io/network/partition/hybrid/tiered/storage/SubpartitionSegmentIdTracker.java Outdated Show resolved Hide resolved
...e/flink/runtime/io/network/partition/hybrid/tiered/storage/SubpartitionSegmentIdTracker.java Outdated Show resolved Hide resolved
...ink/runtime/io/network/partition/hybrid/tiered/storage/SubpartitionSegmentIdTrackerImpl.java Outdated Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
@TanYuxin-tyx TanYuxin-tyx changed the title [FLINK-31640][network] Introduce the segment id tracker for the tiered storage [FLINK-31640][network] Write the accumulated buffers to the right storage tier May 30, 2023
@TanYuxin-tyx TanYuxin-tyx force-pushed the FLINK-31640 branch 3 times, most recently from 93796bf to 258c211 Compare May 31, 2023 13:21
@TanYuxin-tyx
Copy link
Contributor Author

@xintongsong Thanks for the careful review. I have addressed the comments with two fix-up commits.

xintongsong
xintongsong approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@xintongsong xintongsong left a comment

Choose a reason for hiding this comment

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

LGTM.

reswqa
reswqa reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@reswqa reswqa left a comment

Choose a reason for hiding this comment

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

Thanks @TanYuxin-tyx for opening this PR, I left some comments, please take a look.

...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
...java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/TierProducerAgent.java Outdated Show resolved Hide resolved
...java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/TierProducerAgent.java Outdated Show resolved Hide resolved
...java/org/apache/flink/runtime/io/network/partition/hybrid/tiered/tier/TierProducerAgent.java Outdated Show resolved Hide resolved
...he/flink/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClient.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
reswqa
reswqa reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@reswqa reswqa left a comment

Choose a reason for hiding this comment

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

Thanks @TanYuxin-tyx for opening this PR, I left some comments, please take a look.

@TanYuxin-tyx TanYuxin-tyx force-pushed the FLINK-31640 branch 2 times, most recently from 7438a7d to 9efd159 Compare June 2, 2023 07:44
@TanYuxin-tyx
Copy link
Contributor Author

@reswqa Thanks for helping review, I have addressed the comments. Could you help take a look again?
Due to the significant code changes in the test code, I didn't use a fix-up mode to fix it.

@TanYuxin-tyx TanYuxin-tyx force-pushed the FLINK-31640 branch 2 times, most recently from 28cc6f4 to ef797df Compare June 2, 2023 08:13
reswqa
reswqa reviewed Jun 2, 2023
View reviewed changes
...va/org/apache/flink/runtime/io/network/partition/hybrid/tiered/TestingTierProducerAgent.java Outdated Show resolved Hide resolved
...va/org/apache/flink/runtime/io/network/partition/hybrid/tiered/TestingTierProducerAgent.java Outdated Show resolved Hide resolved
...va/org/apache/flink/runtime/io/network/partition/hybrid/tiered/TestingTierProducerAgent.java Outdated Show resolved Hide resolved
...va/org/apache/flink/runtime/io/network/partition/hybrid/tiered/TestingTierProducerAgent.java Outdated Show resolved Hide resolved
...va/org/apache/flink/runtime/io/network/partition/hybrid/tiered/TestingTierProducerAgent.java Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Outdated Show resolved Hide resolved
...link/runtime/io/network/partition/hybrid/tiered/storage/TieredStorageProducerClientTest.java Show resolved Hide resolved
reswqa
reswqa approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@reswqa reswqa left a comment

Choose a reason for hiding this comment

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

Thanks @TanYuxin-tyx for the quick update!👍🏻 LGTM, +1 for merging.

@TanYuxin-tyx
Copy link
Contributor Author

I rebased on the master branch to trigger tests.

@reswqa reswqa merged commit daf476f into apache:master Jun 5, 2023
@TanYuxin-tyx
Copy link
Contributor Author

Thanks @xintongsong and @reswqa for the careful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xintongsong xintongsong xintongsong approved these changes

@reswqa reswqa reswqa approved these changes

Assignees
No one assigned
Labels
component=Runtime/Network
Projects
None yet
Milestone
No milestone
4 participants
@TanYuxin-tyx @flinkbot @xintongsong @reswqa