mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [mesos] asekretenko commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Date Thu, 10 Sep 2020 15:59:14 GMT

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:
users@infra.apache.org



Mime
View raw message