[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/mesos/pull/248


---

[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...

[jpeach]

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 reregisterSlave(
 const vector& frameworkInfos)
 {
   hashset frameworkIDs;
-  hashset executorIDs;
+
+  typedef pair IDPair;
+  hashset executorIDs;
--- End diff --

Mesos doesn't use convenience typedefs.
```C
hashset> executorIDs
```

In subsequent lines, I believe that this is ok:
```
auto id = std::make_pair(executor.framework_id(), executor.executor_id());
```


---

[GitHub] mesos pull request #248: Fix master validation, incorrect detection of dup E...

[jpeach]

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 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 pull request #248: Fix master validation, incorrect detection of dup E...

[jdef]

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 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 pull request #248: Fix master validation, incorrect detection of dup E...

[jdef]

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 
Date:   2017-11-03T13:37:18Z

Fix master validation, incorrect detection of dup ExecutorID.




---