Re: Review Request 42181: Added a process ID argument to Slave constructor.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42181/#review117661
---


Ship it!




Ship It!

- Vinod Kone


On Feb. 3, 2016, 2:31 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42181/
> ---
> 
> (Updated Feb. 3, 2016, 2:31 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing `Slave` constructor to take in the process 
> ID string as an argument. This is necessary for testing recovery of HTTP 
> based executors. (Previously, every invocation of `StartSlave` used to 
> generate a new ID via `ID::generate`). There was no process delegate set via 
> `process::initialize` that led to problems for the existing HTTP based 
> executor to connect back to the slave.
> 
> Also, modified the tests to introduce a new `StartSlave` function that takes 
> this process ID as argument.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
>   src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
>   src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
>   src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
>   src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488 
>   src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 
> 
> Diff: https://reviews.apache.org/r/42181/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42181: Added a process ID argument to Slave constructor.

[Anand Mazumdar]

> On Feb. 2, 2016, 10:05 p.m., Vinod Kone wrote:
> > src/tests/cluster.cpp, lines 409-430
> > 
> >
> > instead of doing if else blocks can you use a ternary operator for the 
> > 1st argument in the constructor?
> > 
> > ```
> > slave.slave = new Slave::Slave(
> >   id.isSome() ? id.get() : process::ID::generate("slave"),
> >   ...
> >   ...);
> > ```
> > 
> > infact instead of declaring an overloaded constructor in slave.hpp and 
> > chaining constructors, can you make 'id' an argument with default value?
> > 
> > ```
> > Slave(...,
> >   const std::string& id = process::ID::generate("slave"))
> > ```
> > 
> > Thoughts?

As per our discussion, modified the existing `Slave` constructor to take an 
"id" as argument. Also, cleaned up other places in code to use this new 
constructor.


- Anand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42181/#review117492
---


On Feb. 3, 2016, 2:31 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42181/
> ---
> 
> (Updated Feb. 3, 2016, 2:31 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing `Slave` constructor to take in the process 
> ID string as an argument. This is necessary for testing recovery of HTTP 
> based executors. (Previously, every invocation of `StartSlave` used to 
> generate a new ID via `ID::generate`). There was no process delegate set via 
> `process::initialize` that led to problems for the existing HTTP based 
> executor to connect back to the slave.
> 
> Also, modified the tests to introduce a new `StartSlave` function that takes 
> this process ID as argument.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
>   src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
>   src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
>   src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
>   src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488 
>   src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 
> 
> Diff: https://reviews.apache.org/r/42181/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42181: Added a process ID argument to Slave constructor.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42181/
---

(Updated Feb. 3, 2016, 2:31 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


Summary (updated)
-

Added a process ID argument to Slave constructor.


Bugs: MESOS-4255
https://issues.apache.org/jira/browse/MESOS-4255


Repository: mesos


Description (updated)
---

This change modifies the existing `Slave` constructor to take in the process ID 
string as an argument. This is necessary for testing recovery of HTTP based 
executors. (Previously, every invocation of `StartSlave` used to generate a new 
ID via `ID::generate`). There was no process delegate set via 
`process::initialize` that led to problems for the existing HTTP based executor 
to connect back to the slave.

Also, modified the tests to introduce a new `StartSlave` function that takes 
this process ID as argument.


Diffs (updated)
-

  src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
  src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
  src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
  src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
  src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

Diff: https://reviews.apache.org/r/42181/diff/


Testing
---

make check


Thanks,

Anand Mazumdar