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-20864][runtime] Apply exactly matching for fine-rained resource profiles #14612

Closed
wants to merge 2 commits into from

Conversation

KarmaGYZ
Copy link
Contributor

@KarmaGYZ KarmaGYZ commented Jan 12, 2021

What is the purpose of the change

This PR proposed to replace the loose matching rules with the following exact matching rules.

  • An unspecified requirement (ResourceProfile::UNKNOWN) can only be fulfilled by a TM's default slot resource.
  • A specified requirement can only be fulfilled by a resource that is equal to itself.

Brief change log

  • Introduce DEFAULT resource profile which represents the default slot profile of a TM and matches the UNKNOWN requirement.
  • Fix the ResourceProfile#isMatching. It's now only used in matching requirements with resources.
  • Introduce ResourceProfile#isBiggerThan, which is used to compare two resource capacities.

@flinkbot
Copy link
Collaborator

flinkbot commented Jan 12, 2021

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit bf9637f (Fri May 28 07:06:50 UTC 2021)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@flinkbot
Copy link
Collaborator

flinkbot commented Jan 12, 2021

CI report:

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

@xintongsong
Copy link
Contributor

xintongsong commented Jan 12, 2021

@KarmaGYZ
I'm not entirely sure about introducing the concept DEFAULT and changing the matching rules for UNKNOWN to be only matched by DEFAULT.

First of all, it complicates the concepts. Currently, ResourceProfile is already complex and hard for new comers to understand.

  • It is used for describing both requirement and resource.
  • UNKNOWN can only be used for requirement
  • ANY can only be used for resource
  • isMatching can only be called as resource.isMatching(requirement)

I think we might want to have two separate classes ResourceRequirement and ResourceCapacity for the above two concepts, and make ResourceProfile a field of them that purely describes the resource value (like primitive types or MemorySize). In this way, we can have special values like ResourceRequirement#UNKNOWN and ResourceCapacity#ANY whose ResourceProfile field is null. However, this is not necessary for this PR. We can open a separate jira ticket to discuss it.

I see how matching unknown requests only to the default slots may help in fine-grained resource management. When there are both fine-grained and unknown requests, the fine-grained requests may create slots with very different resources. Once these slots are reused for the unknown requirements, performance of tasks with unknown requirements could be very unstable, depending on how many resource the reused slot has.

However, I would consider having fine-grained and unknown resource requirements together as a follow-up improvement. I'd rather not touching the matching rules for the unknown requirements for a future improvement, at the risk that the existing corse-grained resource management might also be affected.

@KarmaGYZ
Copy link
Contributor Author

Having two separate classes ResourceRequirement and ResourceCapacity is my future plan. I agree that this effort is out of the scope of this PR.

For the matching logic, I agree that we can treat fine-grained and unknown resource requirements together as a follow-up improvement at the moment. I'll prepare an update soon.

@KarmaGYZ
Copy link
Contributor Author

Thanks for the review, @xintongsong . PR updated.

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 @KarmaGYZ.
The changes LGTM, expect for the message of the hotfix commit is a bit less descriptive, which I will fix myself while merging.

@xintongsong xintongsong changed the title [FLINK-20864][runtime] Introduce the DEFAULT resource profile for the… [FLINK-20864][runtime] Apply exactly matching for fine-rained resource profiles Jan 13, 2021
@KarmaGYZ KarmaGYZ deleted the FLINK-20864 branch March 24, 2021 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants