You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2020/09/10 15:14:59 UTC
[GitHub] [mesos] Lqp1 opened a new pull request #367: Expose hierarchical allocator recovery parameters as master flags
Lqp1 opened a new pull request #367:
URL: https://github.com/apache/mesos/pull/367
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
----------------------------------------------------------------
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
[GitHub] [mesos] Lqp1 commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486892113
##########
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:
You're right; I used your sentence it seems fine to me too.
----------------------------------------------------------------
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
[GitHub] [mesos] asekretenko commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
asekretenko commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486441078
##########
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",
Review comment:
`allocator_agent_recovery_factor`? (see my comment on the word "hierarchical" in `allocator_recovery_timeout`)
----------------------------------------------------------------
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
[GitHub] [mesos] asekretenko commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
asekretenko commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486441078
##########
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",
Review comment:
`allocator_agent_recovery_factor`? (see my comment on the word "hierarchical" below)
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
Is it possible to just use these values from `options`? Why keep a copy?
----------------------------------------------------------------
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
[GitHub] [mesos] Lqp1 commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486925668
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
----------------------------------------------------------------
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
[GitHub] [mesos] Lqp1 commented on pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on pull request #367:
URL: https://github.com/apache/mesos/pull/367#issuecomment-697176988
Thanks!
----------------------------------------------------------------
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
[GitHub] [mesos] Lqp1 commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486892113
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
----------------------------------------------------------------
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
[GitHub] [mesos] asekretenko commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [mesos] Lqp1 commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486892113
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
##########
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:
You're right; I used your sentence it seems fine to me too.
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
##########
File path: include/mesos/allocator/allocator.hpp
##########
@@ -28,6 +28,8 @@
#include <mesos/quota/quota.hpp>
+#include <master/constants.hpp>
Review comment:
So the flags defined for master will depend on `include/allocator/allocator.hpp` header (instead of the more generic `constants.hpp`? Is that ok?
----------------------------------------------------------------
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
[GitHub] [mesos] Lqp1 commented on a change in pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
Lqp1 commented on a change in pull request #367:
URL: https://github.com/apache/mesos/pull/367#discussion_r486893013
##########
File path: src/master/allocator/mesos/hierarchical.hpp
##########
@@ -755,6 +755,8 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess
// Recovery data.
Option<int> expectedAgentCount;
+ float agentRecoveryFactor;
Review comment:
My bad I reworked my patch and forgot to clean this.
----------------------------------------------------------------
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
[GitHub] [mesos] asfgit closed pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #367:
URL: https://github.com/apache/mesos/pull/367
----------------------------------------------------------------
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
[GitHub] [mesos] asfgit closed pull request #367: Expose hierarchical allocator recovery parameters as master flags
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #367:
URL: https://github.com/apache/mesos/pull/367
----------------------------------------------------------------
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