Skip to content

Expose hierarchical allocator recovery parameters as master flags#367

Closed
Lqp1 wants to merge 3 commits intoapache:masterfrom
criteo-forks:allocator-flags-pr
Closed

Expose hierarchical allocator recovery parameters as master flags#367
Lqp1 wants to merge 3 commits intoapache:masterfrom
criteo-forks:allocator-flags-pr

Conversation

@Lqp1
Copy link
Contributor

@Lqp1 Lqp1 commented Sep 10, 2020

Hello,

We needed to expose two parameters of the hierarchical allocator recovery options as master flags. Are you interested in such a PR?

We needed to tweak those values, and rebuilding mesos + deploy it again each time was not very time saving. These flags can be used to have per DC parameters also.

It's rather simple but if you have any input on how I can improve it, don't hesitate.

Br

@Lqp1 Lqp1 marked this pull request as ready for review September 10, 2020 15:14
Copy link
Member

@asekretenko asekretenko 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 addressing this TODO; might be useful to other users running large clusters.

Overall looks good; there is a number of minor issues to be addressed before merging this (see below).


#include <mesos/quota/quota.hpp>

#include <master/constants.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

The definitions in the public allocator interface must not depend on the internal ones.
I.e. the headers in include/ should be usable without ones from src/.

Is it possible to define these constants here, either in this header or in a some other header (in the worst case, in a new one)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the flags defined for master will depend on include/allocator/allocator.hpp header (instead of the more generic constants.hpp? Is that ok?

* use double instead of float
* renamed vars and const
* removed unnecessary attributes in hierarchical allocator
@Lqp1 Lqp1 requested a review from asekretenko September 16, 2020 14:03
Copy link
Member

@asekretenko asekretenko left a comment

Choose a reason for hiding this comment

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

Thanks!
Looks good.
I'll merge this, looks like my recent additions to options cause a conflict.

@asfgit asfgit closed this in 098315f Sep 22, 2020
@Lqp1
Copy link
Contributor Author

Lqp1 commented Sep 23, 2020

Thanks!

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.

2 participants