Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-19 Thread haosdent huang


> On April 19, 2016, 3:37 p.m., Neil Conway wrote:
> > Can we update the docs to describe this behavior? e.g., add a note to 
> > https://mesos.apache.org/documentation/latest/endpoints/ describing the 
> > redirect behavior -- I suppose it is worth adding a note to every master 
> > endpoint's doc page describing the redirect behavior, as well as a note to 
> > the "Master Endpoints" section of the top-level endpoints doc. Might also 
> > be worth adding a note to the "Operational Guide" or "High Availability" 
> > pages. We also need to update the docs for the "master/redirect/" endpoint.
> > 
> > This change requires one or more test cases. For example: (a) normal 
> > redirect case (b) return ServiceUnavailable when there is no leading master 
> > (c) avoiding redirect loop for `/redirect` endpoint.

Thank you very much for your review. Let me update.


- haosdent


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


On April 19, 2016, 1:55 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34646/
> ---
> 
> (Updated April 19, 2016, 1:55 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
> Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we redirect to the leader master in those http
> endpoints which depend on master elected status if current master is
> not a leader.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a9cb99a92ff5a783e719df5e5cfb6e8301241df9 
> 
> Diff: https://reviews.apache.org/r/34646/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> when current master is not a leader, it would redirect to the leader master.
> 
> ```
> $ curl -i http://master1:5050/master/tasks.json
> HTTP/1.1 307 Temporary Redirect
> Date: Mon, 01 Jun 2015 06:30:08 GMT
> Location: http://master2:5050//master/tasks.json
> Content-Length: 0
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-19 Thread Neil Conway

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



Can we update the docs to describe this behavior? e.g., add a note to 
https://mesos.apache.org/documentation/latest/endpoints/ describing the 
redirect behavior -- I suppose it is worth adding a note to every master 
endpoint's doc page describing the redirect behavior, as well as a note to the 
"Master Endpoints" section of the top-level endpoints doc. Might also be worth 
adding a note to the "Operational Guide" or "High Availability" pages. We also 
need to update the docs for the "master/redirect/" endpoint.

This change requires one or more test cases. For example: (a) normal redirect 
case (b) return ServiceUnavailable when there is no leading master (c) avoiding 
redirect loop for `/redirect` endpoint.


src/master/http.cpp (line 345)


"a leader" => "the leader", here and below.



src/master/http.cpp (line 1017)


`"Redirecting request for " << request.url << " to the leading master " << 
hostname.get()`


- Neil Conway


On April 19, 2016, 1:55 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34646/
> ---
> 
> (Updated April 19, 2016, 1:55 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
> Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we redirect to the leader master in those http
> endpoints which depend on master elected status if current master is
> not a leader.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a9cb99a92ff5a783e719df5e5cfb6e8301241df9 
> 
> Diff: https://reviews.apache.org/r/34646/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> when current master is not a leader, it would redirect to the leader master.
> 
> ```
> $ curl -i http://master1:5050/master/tasks.json
> HTTP/1.1 307 Temporary Redirect
> Date: Mon, 01 Jun 2015 06:30:08 GMT
> Location: http://master2:5050//master/tasks.json
> Content-Length: 0
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-18 Thread Mesos ReviewBot

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



Patch looks great!

Reviews applied: [34646]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 19, 2016, 1:55 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34646/
> ---
> 
> (Updated April 19, 2016, 1:55 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
> Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we redirect to the leader master in those http
> endpoints which depend on master elected status if current master is
> not a leader.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a9cb99a92ff5a783e719df5e5cfb6e8301241df9 
> 
> Diff: https://reviews.apache.org/r/34646/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> when current master is not a leader, it would redirect to the leader master.
> 
> ```
> $ curl -i http://master1:5050/master/tasks.json
> HTTP/1.1 307 Temporary Redirect
> Date: Mon, 01 Jun 2015 06:30:08 GMT
> Location: http://master2:5050//master/tasks.json
> Content-Length: 0
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-18 Thread haosdent huang

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

(Updated April 19, 2016, 1:55 a.m.)


Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
Neil Conway, and Vinod Kone.


Changes
---

Update comment for `redirect`.


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


Repository: mesos


Description
---

In this changes, we redirect to the leader master in those http
endpoints which depend on master elected status if current master is
not a leader.


Diffs (updated)
-

  src/master/http.cpp a9cb99a92ff5a783e719df5e5cfb6e8301241df9 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-18 Thread haosdent huang


> On April 18, 2016, 9:18 p.m., Vinod Kone wrote:
> > LGTM overall. Just wondering if this is considered a backwards incompatible 
> > change for operators/tools that did not expect a redirect previously. Can 
> > you send an email to the dev/user list if this is an OK change to make 
> > without a deprecation cycle? Another option would be to do this change in 
> > the v1 operator API and keep the old semantics with the current operator 
> > API.

Got it, I would send an email shortly.


- haosdent


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


On April 16, 2016, 5:15 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34646/
> ---
> 
> (Updated April 16, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
> Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we redirect to the leader master in those http
> endpoints which depend on master elected status if current master is
> not a leader.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d83ccd394c688803369034e22de79b40277a3a7c 
> 
> Diff: https://reviews.apache.org/r/34646/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> when current master is not a leader, it would redirect to the leader master.
> 
> ```
> $ curl -i http://master1:5050/master/tasks.json
> HTTP/1.1 307 Temporary Redirect
> Date: Mon, 01 Jun 2015 06:30:08 GMT
> Location: http://master2:5050//master/tasks.json
> Content-Length: 0
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-18 Thread Vinod Kone

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



LGTM overall. Just wondering if this is considered a backwards incompatible 
change for operators/tools that did not expect a redirect previously. Can you 
send an email to the dev/user list if this is an OK change to make without a 
deprecation cycle? Another option would be to do this change in the v1 operator 
API and keep the old semantics with the current operator API.


src/master/http.cpp (lines 1027 - 1030)


Why strip the url.path in this case?


- Vinod Kone


On April 16, 2016, 5:15 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34646/
> ---
> 
> (Updated April 16, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Cody Maloney, Ian Downes, 
> Neil Conway, and Vinod Kone.
> 
> 
> Bugs: MESOS-1865
> https://issues.apache.org/jira/browse/MESOS-1865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we redirect to the leader master in those http
> endpoints which depend on master elected status if current master is
> not a leader.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d83ccd394c688803369034e22de79b40277a3a7c 
> 
> Diff: https://reviews.apache.org/r/34646/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> when current master is not a leader, it would redirect to the leader master.
> 
> ```
> $ curl -i http://master1:5050/master/tasks.json
> HTTP/1.1 307 Temporary Redirect
> Date: Mon, 01 Jun 2015 06:30:08 GMT
> Location: http://master2:5050//master/tasks.json
> Content-Length: 0
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2016-04-16 Thread haosdent huang

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

(Updated April 16, 2016, 5:01 p.m.)


Review request for mesos and Adam B.


Changes
---

Rebase.


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


Repository: mesos


Description (updated)
---

In this changes, we redirect to the leader master in those http
endpoints which depend on master elected status if current master is
not a leader.


Diffs (updated)
-

  src/master/http.cpp d83ccd394c688803369034e22de79b40277a3a7c 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-03 Thread haosdent huang

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

(Updated July 3, 2015, 2:24 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs (updated)
-

  src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 
  src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-02 Thread haosdent huang


 On June 30, 2015, 3:37 a.m., Adam B wrote:
  src/master/http.cpp, line 424
  https://reviews.apache.org/r/34646/diff/5/?file=975677#file975677line424
 
  Is there a way you could reuse all or part of Http::redirect()? Seems 
  like a lot of copy/paste.

I add a condition in redirect method to return different result.


- haosdent


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


On July 2, 2015, 11:24 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated July 2, 2015, 11:24 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 
   src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-02 Thread haosdent huang

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

(Updated July 2, 2015, 11:24 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs (updated)
-

  src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 
  src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-02 Thread haosdent huang

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

(Updated July 2, 2015, 11:22 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs (updated)
-

  src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 
  src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-01 Thread Adam B


 On June 1, 2015, 1:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295
 
 haosdent huang wrote:
 @adam-mesos Thank you very much! I miss them when I read the unit tests. 
 Let me try to add a unit test.
 
 haosdent huang wrote:
 @adam-mesos In `MasterAllocatorTest.FrameworkReregistersFirst` and 
 `MasterAllocatorTest.SlaveReregistersFirst`, before them start the second 
 Master, it need call `this-ShutdownMasters();`. So there is only one master 
 when test. But this test case need two alive masters, I try to start two 
 masters, but it failed. So we have to change some thing make it possible to 
 start two alive masters in unit tests.
 
 Adam B wrote:
 Okay. Sounds complicated. Just make sure you manually test. I'll drop 
 this issue.
 
 haosdent huang wrote:
 @adam-mesos I try to find the reason why could not start two Master 
 instances in the test case serveral days ago. I think I am nearly to reach 
 the cause now. Let me reopen this issue and keep writing a test case for this.
 
 haosdent huang wrote:
 @adam-mesos, sorry for not update this for a long time. Currently, in 
 [ProcessManager::spawn](https://github.com/apache/mesos/blob/0.22.1-rc6/3rdparty/libprocess/src/process.cpp#L2129-L2131),
  we would check the process id exists or not.
 ```
 if (processes.count(process-pid.id)  0) {
   return UPID();
 } else {
   processes[process-pid.id] = process;
 }
 ```
 If it is exists, it would not spawn the processes.
 For master, because we use a fixed pid.id. The above code pass master 
 as a fixed pid.id. So could not start two masters in our test cases now.
 ```
 Master::Master(
 Allocator* _allocator,
 Registrar* _registrar,
 Repairer* _repairer,
 Files* _files,
 MasterContender* _contender,
 MasterDetector* _detector,
 const OptionAuthorizer* _authorizer,
 const Optionshared_ptrRateLimiter _slaveRemovalLimiter,
 const Flags _flags)
   : ProcessBase(master),
 ```
 But Slave use process::ID::generate() to generate id. The result of slave 
 id would be like this: slave(1), slave(2) and so on.
 ```
 Slave::Slave(const slave::Flags _flags,
  MasterDetector* _detector,
  Containerizer* _containerizer,
  Files* _files,
  GarbageCollector* _gc,
  StatusUpdateManager* _statusUpdateManager,
  ResourceEstimator* _resourceEstimator,
  QoSController* _qosController)
   : ProcessBase(process::ID::generate(slave)),
 ```
 Let me change some code in tests/cluster.hpp and tests/clusters.cpp to 
 make start two masters possible.

This sounds like a non-trivial amount of effort. Let's not go too far down the 
rabbit-hole just for testing this patch.
Since this is a broader testing requirement, could you create a separate JIRA 
for allowing tests to use two masters?


- Adam


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


On June 1, 2015, 8:07 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 8:07 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 

Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-01 Thread haosdent huang


 On June 1, 2015, 8:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295
 
 haosdent huang wrote:
 @adam-mesos Thank you very much! I miss them when I read the unit tests. 
 Let me try to add a unit test.
 
 haosdent huang wrote:
 @adam-mesos In `MasterAllocatorTest.FrameworkReregistersFirst` and 
 `MasterAllocatorTest.SlaveReregistersFirst`, before them start the second 
 Master, it need call `this-ShutdownMasters();`. So there is only one master 
 when test. But this test case need two alive masters, I try to start two 
 masters, but it failed. So we have to change some thing make it possible to 
 start two alive masters in unit tests.
 
 Adam B wrote:
 Okay. Sounds complicated. Just make sure you manually test. I'll drop 
 this issue.
 
 haosdent huang wrote:
 @adam-mesos I try to find the reason why could not start two Master 
 instances in the test case serveral days ago. I think I am nearly to reach 
 the cause now. Let me reopen this issue and keep writing a test case for this.
 
 haosdent huang wrote:
 @adam-mesos, sorry for not update this for a long time. Currently, in 
 [ProcessManager::spawn](https://github.com/apache/mesos/blob/0.22.1-rc6/3rdparty/libprocess/src/process.cpp#L2129-L2131),
  we would check the process id exists or not.
 ```
 if (processes.count(process-pid.id)  0) {
   return UPID();
 } else {
   processes[process-pid.id] = process;
 }
 ```
 If it is exists, it would not spawn the processes.
 For master, because we use a fixed pid.id. The above code pass master 
 as a fixed pid.id. So could not start two masters in our test cases now.
 ```
 Master::Master(
 Allocator* _allocator,
 Registrar* _registrar,
 Repairer* _repairer,
 Files* _files,
 MasterContender* _contender,
 MasterDetector* _detector,
 const OptionAuthorizer* _authorizer,
 const Optionshared_ptrRateLimiter _slaveRemovalLimiter,
 const Flags _flags)
   : ProcessBase(master),
 ```
 But Slave use process::ID::generate() to generate id. The result of slave 
 id would be like this: slave(1), slave(2) and so on.
 ```
 Slave::Slave(const slave::Flags _flags,
  MasterDetector* _detector,
  Containerizer* _containerizer,
  Files* _files,
  GarbageCollector* _gc,
  StatusUpdateManager* _statusUpdateManager,
  ResourceEstimator* _resourceEstimator,
  QoSController* _qosController)
   : ProcessBase(process::ID::generate(slave)),
 ```
 Let me change some code in tests/cluster.hpp and tests/clusters.cpp to 
 make start two masters possible.
 
 Adam B wrote:
 This sounds like a non-trivial amount of effort. Let's not go too far 
 down the rabbit-hole just for testing this patch.
 Since this is a broader testing requirement, could you create a separate 
 JIRA for allowing tests to use two masters?

Yes, I think it should be a seperate issue and need a seperate patch. So I just 
need manual test this patch?


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when 

Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-01 Thread haosdent huang


 On June 1, 2015, 8:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295
 
 haosdent huang wrote:
 @adam-mesos Thank you very much! I miss them when I read the unit tests. 
 Let me try to add a unit test.
 
 haosdent huang wrote:
 @adam-mesos In `MasterAllocatorTest.FrameworkReregistersFirst` and 
 `MasterAllocatorTest.SlaveReregistersFirst`, before them start the second 
 Master, it need call `this-ShutdownMasters();`. So there is only one master 
 when test. But this test case need two alive masters, I try to start two 
 masters, but it failed. So we have to change some thing make it possible to 
 start two alive masters in unit tests.
 
 Adam B wrote:
 Okay. Sounds complicated. Just make sure you manually test. I'll drop 
 this issue.
 
 haosdent huang wrote:
 @adam-mesos I try to find the reason why could not start two Master 
 instances in the test case serveral days ago. I think I am nearly to reach 
 the cause now. Let me reopen this issue and keep writing a test case for this.

@adam-mesos, sorry for not update this for a long time. Currently, in 
[ProcessManager::spawn](https://github.com/apache/mesos/blob/0.22.1-rc6/3rdparty/libprocess/src/process.cpp#L2129-L2131),
 we would check the process id exists or not.
```
if (processes.count(process-pid.id)  0) {
  return UPID();
} else {
  processes[process-pid.id] = process;
}
```
If it is exists, it would not spawn the processes.
For master, because we use a fixed pid.id. The above code pass master as a 
fixed pid.id. So could not start two masters in our test cases now.
```
Master::Master(
Allocator* _allocator,
Registrar* _registrar,
Repairer* _repairer,
Files* _files,
MasterContender* _contender,
MasterDetector* _detector,
const OptionAuthorizer* _authorizer,
const Optionshared_ptrRateLimiter _slaveRemovalLimiter,
const Flags _flags)
  : ProcessBase(master),
```
But Slave use process::ID::generate() to generate id. The result of slave id 
would be like this: slave(1), slave(2) and so on.
```
Slave::Slave(const slave::Flags _flags,
 MasterDetector* _detector,
 Containerizer* _containerizer,
 Files* _files,
 GarbageCollector* _gc,
 StatusUpdateManager* _statusUpdateManager,
 ResourceEstimator* _resourceEstimator,
 QoSController* _qosController)
  : ProcessBase(process::ID::generate(slave)),
```
Let me change some code in tests/cluster.hpp and tests/clusters.cpp to make 
start two masters possible.


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-07-01 Thread Adam B


 On June 1, 2015, 1:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295
 
 haosdent huang wrote:
 @adam-mesos Thank you very much! I miss them when I read the unit tests. 
 Let me try to add a unit test.
 
 haosdent huang wrote:
 @adam-mesos In `MasterAllocatorTest.FrameworkReregistersFirst` and 
 `MasterAllocatorTest.SlaveReregistersFirst`, before them start the second 
 Master, it need call `this-ShutdownMasters();`. So there is only one master 
 when test. But this test case need two alive masters, I try to start two 
 masters, but it failed. So we have to change some thing make it possible to 
 start two alive masters in unit tests.
 
 Adam B wrote:
 Okay. Sounds complicated. Just make sure you manually test. I'll drop 
 this issue.
 
 haosdent huang wrote:
 @adam-mesos I try to find the reason why could not start two Master 
 instances in the test case serveral days ago. I think I am nearly to reach 
 the cause now. Let me reopen this issue and keep writing a test case for this.
 
 haosdent huang wrote:
 @adam-mesos, sorry for not update this for a long time. Currently, in 
 [ProcessManager::spawn](https://github.com/apache/mesos/blob/0.22.1-rc6/3rdparty/libprocess/src/process.cpp#L2129-L2131),
  we would check the process id exists or not.
 ```
 if (processes.count(process-pid.id)  0) {
   return UPID();
 } else {
   processes[process-pid.id] = process;
 }
 ```
 If it is exists, it would not spawn the processes.
 For master, because we use a fixed pid.id. The above code pass master 
 as a fixed pid.id. So could not start two masters in our test cases now.
 ```
 Master::Master(
 Allocator* _allocator,
 Registrar* _registrar,
 Repairer* _repairer,
 Files* _files,
 MasterContender* _contender,
 MasterDetector* _detector,
 const OptionAuthorizer* _authorizer,
 const Optionshared_ptrRateLimiter _slaveRemovalLimiter,
 const Flags _flags)
   : ProcessBase(master),
 ```
 But Slave use process::ID::generate() to generate id. The result of slave 
 id would be like this: slave(1), slave(2) and so on.
 ```
 Slave::Slave(const slave::Flags _flags,
  MasterDetector* _detector,
  Containerizer* _containerizer,
  Files* _files,
  GarbageCollector* _gc,
  StatusUpdateManager* _statusUpdateManager,
  ResourceEstimator* _resourceEstimator,
  QoSController* _qosController)
   : ProcessBase(process::ID::generate(slave)),
 ```
 Let me change some code in tests/cluster.hpp and tests/clusters.cpp to 
 make start two masters possible.
 
 Adam B wrote:
 This sounds like a non-trivial amount of effort. Let's not go too far 
 down the rabbit-hole just for testing this patch.
 Since this is a broader testing requirement, could you create a separate 
 JIRA for allowing tests to use two masters?
 
 haosdent huang wrote:
 Yes, I think it should be a seperate issue and need a seperate patch. So 
 I just need manual test this patch?

Manual testing should be fine for now. You can create another follow-up JIRA to 
create a unit test for this, and make that JIRA dependent on this one and the 
multi-master testing JIRA.


- Adam


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


On June 1, 2015, 8:07 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 8:07 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   

Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-30 Thread haosdent huang


 On June 30, 2015, 6:48 p.m., Vinod Kone wrote:
  This is an API change! Before embarking on this change, it's prudent to get 
  feedback from the community on acceptable semantics (e.g., what do popular 
  metric collectors do if they encounter 302 redirect? do they ignore the 
  value or follow the redirect? if the latter, it might result in duplicate 
  collection!). Can you send an email to the dev list?
 
 haosdent huang wrote:
 Sure.Thank you for your review. @vinodkone
 
 Adam B wrote:
 It's important to note that this patch does not redirect on certain 
 master endpoints like /observe, /health and the metrics endpoints, since 
 those were seen as potentially host-specific. Other endpoints like /slaves, 
 /state, /stateSummary, /roles, /tasks, and /teardown were assumed only useful 
 when they hit the leading master. That said, I agree that this is an API 
 change and we should make sure the community is okay with the redirects on 
 some endpoints and not others.

Oh, got it. How about change the fix version of this issue to 0.24? @adam-mesos


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-30 Thread Adam B


 On June 30, 2015, 11:48 a.m., Vinod Kone wrote:
  This is an API change! Before embarking on this change, it's prudent to get 
  feedback from the community on acceptable semantics (e.g., what do popular 
  metric collectors do if they encounter 302 redirect? do they ignore the 
  value or follow the redirect? if the latter, it might result in duplicate 
  collection!). Can you send an email to the dev list?
 
 haosdent huang wrote:
 Sure.Thank you for your review. @vinodkone
 
 Adam B wrote:
 It's important to note that this patch does not redirect on certain 
 master endpoints like /observe, /health and the metrics endpoints, since 
 those were seen as potentially host-specific. Other endpoints like /slaves, 
 /state, /stateSummary, /roles, /tasks, and /teardown were assumed only useful 
 when they hit the leading master. That said, I agree that this is an API 
 change and we should make sure the community is okay with the redirects on 
 some endpoints and not others.
 
 haosdent huang wrote:
 Oh, got it. How about change the fix version of this issue to 0.24? 
 @adam-mesos

Unnecessary. We'll just leave it untargeted and let it land whenever it lands. 
No reason for it to be a blocker for 0.24.


- Adam


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


On June 1, 2015, 8:07 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 8:07 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-30 Thread Vinod Kone

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


This is an API change! Before embarking on this change, it's prudent to get 
feedback from the community on acceptable semantics (e.g., what do popular 
metric collectors do if they encounter 302 redirect? do they ignore the value 
or follow the redirect? if the latter, it might result in duplicate 
collection!). Can you send an email to the dev list?

- Vinod Kone


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-29 Thread Adam B

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


Looks great, thanks! I still wonder if we can remove some of the duplication 
between redirect() and redirectRequestToLeader(). Otherwise just minor feedback.


src/master/http.cpp (line 424)
https://reviews.apache.org/r/34646/#comment142698

Is there a way you could reuse all or part of Http::redirect()? Seems like 
a lot of copy/paste.



src/master/http.cpp (line 427)
https://reviews.apache.org/r/34646/#comment142694

Nit: Comments should end in punctuation.



src/master/http.cpp (lines 429 - 433)
https://reviews.apache.org/r/34646/#comment142695

Please wrap at 80 characters.
Suggested rewording: Current master is not elected as leader, and leader 
information is unavailable. Failed to redirect the request url:   request.url



src/master/http.cpp (line 434)
https://reviews.apache.org/r/34646/#comment142696

Why not a 503 Service Unavailable, meaning The server is currently 
unavailable (because it is overloaded or down for maintenance). Generally, this 
is a temporary state.?



src/master/http.cpp (lines 438 - 441)
https://reviews.apache.org/r/34646/#comment142697

Please wrap at 80 characters.
Suggested rewording: Current master is not elected leader. Redirecting the 
request url   request.url   to the leading master   hostname



src/master/http.cpp (line 454)
https://reviews.apache.org/r/34646/#comment142596

Remove http:, like we're doing for the rest of https/SSL support. See 
https://reviews.apache.org/r/36018/diff



src/master/http.cpp (line 471)
https://reviews.apache.org/r/34646/#comment142699

s/leader master/leading master/g



src/master/master.hpp (line 1096)
https://reviews.apache.org/r/34646/#comment142700

s/Reredirect/Redirect/
s/leader master/leading master/


- Adam B


On June 1, 2015, 8:07 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 8:07 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-07 Thread haosdent huang


 On June 1, 2015, 8:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295
 
 haosdent huang wrote:
 @adam-mesos Thank you very much! I miss them when I read the unit tests. 
 Let me try to add a unit test.

@adam-mesos In `MasterAllocatorTest.FrameworkReregistersFirst` and 
`MasterAllocatorTest.SlaveReregistersFirst`, before them start the second 
Master, it need call `this-ShutdownMasters();`. So there is only one master 
when test. But this test case need two alive masters, I try to start two 
masters, but it failed. So we have to change some thing make it possible to 
start two alive masters in unit tests.


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread haosdent huang

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

(Updated June 1, 2015, 6:35 a.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs
-

  src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 

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


Testing (updated)
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread haosdent huang


 On May 28, 2015, 7:51 a.m., Adam B wrote:
  src/master/http.cpp, line 1044
  https://reviews.apache.org/r/34646/diff/1/?file=971274#file971274line1044
 
  The JIRA suggests that this case should not return OK (200), unless it 
  queries the active master to return the actual tasks list. Otherwise, if 
  it's going to return a blank tasks[] array, then it should return something 
  like a 500 (Internal Server Error), or 307 (Temporary Redirect).

@adam-mesos I update the patch again. Feel sorry to misunderstand the issue 
before. Could you review again? Thank you very much.


- haosdent


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


On June 1, 2015, 6:35 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 6:35 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [34646]

All tests passed.

- Mesos ReviewBot


On June 1, 2015, 6:35 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 6:35 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread haosdent huang

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

(Updated June 1, 2015, 3:07 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Return empty task list when current master is not a leader.


Diffs (updated)
-

  src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
  src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 

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


Testing
---

make check

when current master is not a leader, it would redirect to the leader master.

```
$ curl -i http://master1:5050/master/tasks.json
HTTP/1.1 307 Temporary Redirect
Date: Mon, 01 Jun 2015 06:30:08 GMT
Location: http://master2:5050//master/tasks.json
Content-Length: 0
```


Thanks,

haosdent huang



Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread haosdent huang


 On June 1, 2015, 8:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes

@adam-mesos, I have a newbie problem. In the unit test, do I start a 
multi-masters cluster to verify this? I search the exist unit tests, could not 
find a exist snippet to start a multi-masters cluster.


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread Adam B


 On June 1, 2015, 1:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.

Good question. Our unit test macros are not well documented, so I usually find 
a couple of example tests to copy from. Check out 
MasterAllocatorTest.FrameworkReregistersFirst and 
MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
leader-election between two masters.
https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295


- Adam


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


On June 1, 2015, 8:07 a.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 8:07 a.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang
 




Re: Review Request 34646: Redirect to the leader master when current master is not a leader.

2015-06-01 Thread haosdent huang


 On June 1, 2015, 8:34 a.m., Adam B wrote:
  src/master/http.cpp, line 1038
  https://reviews.apache.org/r/34646/diff/3/?file=975522#file975522line1038
 
  Could I ask you to write a quick unit test for this?
 
 haosdent huang wrote:
 yes
 
 haosdent huang wrote:
 @adam-mesos, I have a newbie problem. In the unit test, do I start a 
 multi-masters cluster to verify this? I search the exist unit tests, could 
 not find a exist snippet to start a multi-masters cluster.
 
 Adam B wrote:
 Good question. Our unit test macros are not well documented, so I usually 
 find a couple of example tests to copy from. Check out 
 MasterAllocatorTest.FrameworkReregistersFirst and 
 MasterAllocatorTest.SlaveReregistersFirst, which actually do failover and 
 leader-election between two masters.
 
 https://github.com/apache/mesos/blob/0.22.1/src/tests/master_allocator_tests.cpp#L1295

@adam-mesos Thank you very much! I miss them when I read the unit tests. Let me 
try to add a unit test.


- haosdent


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


On June 1, 2015, 3:07 p.m., haosdent huang wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34646/
 ---
 
 (Updated June 1, 2015, 3:07 p.m.)
 
 
 Review request for mesos and Adam B.
 
 
 Bugs: MESOS-1865
 https://issues.apache.org/jira/browse/MESOS-1865
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Return empty task list when current master is not a leader.
 
 
 Diffs
 -
 
   src/master/http.cpp 01dbdad9720a9cd30505683ae1117970cda918a0 
   src/master/master.hpp c0cc2933a2cc094401f633df12356bda3d294564 
 
 Diff: https://reviews.apache.org/r/34646/diff/
 
 
 Testing
 ---
 
 make check
 
 when current master is not a leader, it would redirect to the leader master.
 
 ```
 $ curl -i http://master1:5050/master/tasks.json
 HTTP/1.1 307 Temporary Redirect
 Date: Mon, 01 Jun 2015 06:30:08 GMT
 Location: http://master2:5050//master/tasks.json
 Content-Length: 0
 ```
 
 
 Thanks,
 
 haosdent huang