asekretenko commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486435496
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
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)?
##########
File path: src/master/flags.hpp
##########
@@ -81,6 +81,8 @@ class Flags : public virtual logging::Flags
Option<std::string> modulesDir;
std::string authenticators;
std::string allocator;
+ float hierarchical_recovery_factor;
Review comment:
I would suggest leaving `double`, as Mesos code generally uses `double`
and `double` is slightly more difficult to use incorrectly.
Note that `0.80` in the line `constexpr float DEFAULT_AGENT_RECOVERY_FACTOR
= 0.80;` is a **double** literal, not a float.
##########
File path: src/master/flags.cpp
##########
@@ -460,6 +460,18 @@ mesos::internal::master::Flags::Flags()
"load an alternate allocator module using `--modules`.",
DEFAULT_ALLOCATOR);
+ add(&Flags::hierarchical_recovery_factor,
+ "hierarchical_recovery_factor",
+ "Ratio of minimal re-registred agent before sending\n"
Review comment:
"Ratio" usually implies two numbers (ratio of something to something
else), doesn't it?..
Maybe something like "Minimum fraction of known agents re-registered after
leader election for the allocator to start generating offers"?
##########
File path: src/master/flags.cpp
##########
@@ -460,6 +460,18 @@ mesos::internal::master::Flags::Flags()
"load an alternate allocator module using `--modules`.",
DEFAULT_ALLOCATOR);
+ add(&Flags::hierarchical_recovery_factor,
+ "hierarchical_recovery_factor",
+ "Ratio of minimal re-registred agent before sending\n"
+ "offers after a leader re-election",
+ DEFAULT_AGENT_RECOVERY_FACTOR);
+
+ add(&Flags::hierarchical_recovery_timeout,
+ "hierarchical_recovery_timeout",
Review comment:
Maybe `allocator_recovery_timeout`?
In the master code that fills `allocator::Options` nothing implies that the
created allocator is indeed _the_ hierarchical allocator.
Moreover, the existence of recovery interval is not relevant to the fact
that the allocator treats resource roles hierarchically.
##########
File path: src/master/flags.cpp
##########
@@ -460,6 +460,18 @@ mesos::internal::master::Flags::Flags()
"load an alternate allocator module using `--modules`.",
DEFAULT_ALLOCATOR);
+ add(&Flags::hierarchical_recovery_factor,
+ "hierarchical_recovery_factor",
+ "Ratio of minimal re-registred agent before sending\n"
+ "offers after a leader re-election",
+ DEFAULT_AGENT_RECOVERY_FACTOR);
Review comment:
`DEFAULT_ALLOCATOR_AGENT_RECOVERY_FACTOR`?
Looks like the already existing `Flags` code follows the convention that the
default has the same name as the flag, prefixed with `DEFAULT_`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
