Skip to content

Conversation

@dongjoon-hyun
Copy link

No description provided.

val offerAttributes = toAttributeMap(offer.getAttributesList)
matchesAttributeRequirements(slaveOfferConstraints, offerAttributes)
if (blacklist.contains(offer.getHostname)) {
false
Copy link
Author

Choose a reason for hiding this comment

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

This will save many computation on the invalid offers.

Copy link
Owner

@IgorBerman IgorBerman Sep 26, 2019

Choose a reason for hiding this comment

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

I don't mind to add this optimisation, but imo, you can't remove original check. Since they happening in two different places and the code that matches offer is "repeated" at canLaunchTask

Copy link
Author

@dongjoon-hyun dongjoon-hyun Sep 26, 2019

Choose a reason for hiding this comment

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

It sounds like you are saying unmatchedOffers will be evaluated at canLaunchTask. Does it what you mean really? At line 392, we simple do declineUnmatchedOffers(d, unmatchedOffers).

Copy link
Owner

Choose a reason for hiding this comment

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

I mean that even though the offer is matched, then there is code in canLaunchTask that verifies that task can be launched with respect to cpu/memory requirements(so also blacklisting)

Copy link
Author

Choose a reason for hiding this comment

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

I said the computation on the invalid offers (not matched).

This will save many computation on the invalid offers.

Copy link

Choose a reason for hiding this comment

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

I agree with @dongjoon-hyun , seems this would be more efficient. I don't understand the objection

@dongjoon-hyun
Copy link
Author

dongjoon-hyun commented Sep 26, 2019

Since it's already over 20 hours and it seems you disagree, I'll wait for 4 hours from now and make independent PR on the Apache Spark community officially for the wider review. At that time, I'll close this PR.

@IgorBerman IgorBerman merged commit dbe7d18 into IgorBerman:SPARK-19755-Blacklist-is-always-active-for-MesosCoarseGrainedSchedulerBackend Oct 2, 2019
@IgorBerman
Copy link
Owner

@squito, @dongjoon-hyun I'm not against this PR, what I "against" is removing check from canLaunchTask method, but I agree that checking blacklisting in match offers will be better.
anyway I've merged PR(maybe I don't understand something)

@dongjoon-hyun dongjoon-hyun deleted the PR-20640-2 branch November 23, 2019 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants