You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by jdef <gi...@git.apache.org> on 2017/11/03 13:41:54 UTC
[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...
GitHub user jdef opened a pull request:
https://github.com/apache/mesos/pull/248
Fix master validation, incorrect detection of dup ExecutorID.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jdef/mesos jdef_master_validation_reregisterslave
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/mesos/pull/248.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #248
----
commit 27127e7731a40baedcc88af6929525c9f8b1e5e7
Author: James DeFelice <ja...@gmail.com>
Date: 2017-11-03T13:37:18Z
Fix master validation, incorrect detection of dup ExecutorID.
----
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by jdef <gi...@git.apache.org>.
Github user jdef commented on the issue:
https://github.com/apache/mesos/pull/248
@jpeach it looks like you were in here recently, what are your thoughts on this?
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by jdef <gi...@git.apache.org>.
Github user jdef commented on the issue:
https://github.com/apache/mesos/pull/248
https://issues.apache.org/jira/browse/MESOS-8169
---
[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/mesos/pull/248#discussion_r148844153
--- Diff: src/master/validation.cpp ---
@@ -350,12 +353,13 @@ Option<Error> reregisterSlave(
}
if (executor.has_executor_id()) {
- if (executorIDs.contains(executor.executor_id())) {
+ IDPair idpair = IDPair(executor.framework_id(), executor.executor_id());
+ if (executorIDs.contains(idpair)) {
return Error("Executor has a duplicate ExecutorID '" +
stringify(executor.executor_id()) + "'");
--- End diff --
We should update this error message:
```
Error("Framework '" + executor.framework_id() +
'" has a duplicate ExecutorID '" +
stringify(executor.executor_id()) + "'");
```
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by jdef <gi...@git.apache.org>.
Github user jdef commented on the issue:
https://github.com/apache/mesos/pull/248
@abudnik the master.hpp code you cited is already scoped to Framework. I don't think we need to change anything there.
---
[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/mesos/pull/248
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by abudnik <gi...@git.apache.org>.
Github user abudnik commented on the issue:
https://github.com/apache/mesos/pull/248
We should also update here https://github.com/apache/mesos/blob/master/src/master/master.hpp#L2665-L2667
and maybe in other places, where master/agent checks for duplicate executor ids.
---
[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...
Posted by jdef <gi...@git.apache.org>.
Github user jdef commented on a diff in the pull request:
https://github.com/apache/mesos/pull/248#discussion_r148790566
--- Diff: src/master/validation.cpp ---
@@ -350,12 +353,13 @@ Option<Error> reregisterSlave(
}
if (executor.has_executor_id()) {
- if (executorIDs.contains(executor.executor_id())) {
+ IDPair fep = IDPair(executor.framework_id(), executor.executor_id());
--- End diff --
already got feedback that `fep` is probably not a good choice for a name here
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/mesos/pull/248
@jdef Good find! Can you please make a JIRA for this?
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by abudnik <gi...@git.apache.org>.
Github user abudnik commented on the issue:
https://github.com/apache/mesos/pull/248
Should we also update comment https://github.com/apache/mesos/blob/master/include/mesos/mesos.proto#L78-L82 ?
---
[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/mesos/pull/248#discussion_r148843748
--- Diff: src/master/validation.cpp ---
@@ -298,7 +299,9 @@ Option<Error> reregisterSlave(
const vector<FrameworkInfo>& frameworkInfos)
{
hashset<FrameworkID> frameworkIDs;
- hashset<ExecutorID> executorIDs;
+
+ typedef pair<FrameworkID, ExecutorID> IDPair;
+ hashset<IDPair> executorIDs;
--- End diff --
Mesos doesn't use convenience typedefs.
```C
hashset<pair<FrameworkID, ExecutorID>> executorIDs
```
In subsequent lines, I believe that this is ok:
```
auto id = std::make_pair(executor.framework_id(), executor.executor_id());
```
---
[GitHub] mesos issue #248: Fix master validation, incorrect detection of dup Executor...
Posted by abudnik <gi...@git.apache.org>.
Github user abudnik commented on the issue:
https://github.com/apache/mesos/pull/248
If some framework relies on the assumption that there shouldn't be multiple executors running with the same executor_id on the same agent, then this change might lead to bugs.
It would be great to post an email into dev/user-lists regarding this change.
---