Re: Review Request 37303: Moved scheduler library to http

2015-08-13 Thread Vinod Kone

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

Ship it!


Looks good modulo minor fixes. I'll fix them and commit for you.


src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment150090

indentation.



src/scheduler/scheduler.cpp (line 393)
https://reviews.apache.org/r/37303/#comment150091

No comment here?



src/scheduler/scheduler.cpp (line 412)
https://reviews.apache.org/r/37303/#comment150092

indentation.


- Vinod Kone


On Aug. 13, 2015, 5:39 a.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 13, 2015, 5:39 a.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar
 




Re: Review Request 37303: Moved scheduler library to http

2015-08-12 Thread Anand Mazumdar

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

(Updated Aug. 12, 2015, 10:26 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This changes moves scheduler library to http. The change to remove auth + 
install,receive handlers are in other reviews.


Diffs (updated)
-

  src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
  src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 

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


Testing
---

make check


Thanks,

Anand Mazumdar



Re: Review Request 37303: Moved scheduler library to http

2015-08-12 Thread Vinod Kone

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



src/common/http.hpp (line 51)
https://reviews.apache.org/r/37303/#comment150063

kill this.



src/scheduler/scheduler.cpp (line 257)
https://reviews.apache.org/r/37303/#comment150077

make sure to close the http connection here to avoid accidentally sending 
non-subscribe calls to a new master.



src/scheduler/scheduler.cpp (line 321)
https://reviews.apache.org/r/37303/#comment150064

add a comment on when this is possible.



src/scheduler/scheduler.cpp (line 342)
https://reviews.apache.org/r/37303/#comment150066

s/keepReading/read/



src/scheduler/scheduler.cpp (line 351)
https://reviews.apache.org/r/37303/#comment150067

s/a/unexpected/



src/scheduler/scheduler.cpp (line 353)
https://reviews.apache.org/r/37303/#comment150074

when can we get here? can you add a comment?

per our disccussion, this should call error()?



src/scheduler/scheduler.cpp (line 359)
https://reviews.apache.org/r/37303/#comment150078

indentation.



src/scheduler/scheduler.cpp (line 365)
https://reviews.apache.org/r/37303/#comment150068

s/oldReader/reader/



src/scheduler/scheduler.cpp (line 383)
https://reviews.apache.org/r/37303/#comment150069

this could happen during master failover too. so let's not send an error()



src/scheduler/scheduler.cpp (line 396)
https://reviews.apache.org/r/37303/#comment150071

indent by 2 spaces.

s/WARNING/ERROR/

this should call error().


- Vinod Kone


On Aug. 12, 2015, 10:26 p.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 12, 2015, 10:26 p.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar
 




Re: Review Request 37303: Moved scheduler library to http

2015-08-12 Thread Anand Mazumdar

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

(Updated Aug. 12, 2015, 6:51 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Moved Pipe::reader, decoder to a separate struct Connection + Other review 
comments from Ben


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


Repository: mesos


Description
---

This changes moves scheduler library to http. The change to remove auth + 
install,receive handlers are in other reviews.


Diffs (updated)
-

  src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
  src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 

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


Testing
---

make check


Thanks,

Anand Mazumdar



Re: Review Request 37303: Moved scheduler library to http

2015-08-12 Thread Anand Mazumdar


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  Sorry for not elaborating on all of these, I added some more explanations 
  here. Main thing is cleaning up the read loop and figuring out the callback 
  semantics (do we need to revisit 'connected' / 'disconnected'?).

Let's keep the callback behavior the same as what it was before. We can decide 
to change the semantics if we feel the need later in a separate patch.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 117
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line117
 
  Sorry, let me elaborate further. I mentioned not having this because 
  headers requires non-local reasoning when reading the request sending 
  code:
  
  ```
// Send a streaming request for Subscribe call.
response = process::http::streaming::post(
master.get(),
api/v1/scheduler,
headers, // non-local
body,
stringify(contentType));

  vs.
  
// Send a streaming request for Subscribe call.
response = process::http::streaming::post(
master.get(),
api/v1/scheduler,
{{Accept, stringify(contentType)}}, // local
body,
stringify(contentType));
  ```
  
  We've generally found local reasoning makes code easier to read (a.k.a. 
  readable). Also, keep in mind that in the future you'll be able to send a 
  Request directly, so it would become:
  
  ```
  Request request;
  ...
  request.headers[Accept] = stringify(contentType);
  
  response = process::http::streaming::request(request);
  ```
  
  The other concern is that headers is not necessarily going to remain 
  constant like this in the future (e.g. if we add the notion of a session 
  header). Make sense?

Anyways , this won't compile. I added it as a const local variable for now.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/common/http.hpp, lines 56-57
  https://reviews.apache.org/r/37303/diff/4/?file=1038062#file1038062line56
 
  You can `return stream  ...;` in a single statement.

Looked more readable this way. Made the change though.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 329
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line329
 
  The reason I ask to flip this is we've generally not used yoda style 
  comparisons, e.g.
  
  ```
  size()  1
  
  rather than
  
  1  size()
  ```
  
  Can you do a sweep in the code you've introduced here?

Fixed. I guess we already have -Wall,-Werror in our code. So my concerns around 
accidently assigning in if statements don't make much sense here.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 330
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line330
 
  Flip the comparison here as well. Be sure to do a sweep.

Done. CHECK_EQ though normally have syntax like CHECK_EQ(expected, actual)


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 354-362
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line354
 
  I suggested the struct because passing around the recodio reader 
  independently of the pipe reader seems to be not that intuitive, was hoping 
  to see them stored together inside an OptionConnection rather than having 
  the OptionPipe::Reader and recordio::Reader stored separately. Make more 
  sense?
  
  Also, can we use recordio::Reader to be more explicit about this type?

Done, Moved it to a struct Connection. Also since RecordIO::Reader is a process 
now. Saving it in a process::Owned member field inside the struct.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 387-394
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line387
 
  Ok, let's chat about this what to do here, might need to revisit the 
  callbacks here now that we have http. But this shouldn't be an error since 
  it's just disconnection. If we call disconnected here though, seems that we 
  need to immediately call connected if there is still a master available, 
  otherwise we might hang around when we should be retrying.

Removed the error event. Just logging an error for now.


 On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 388-389
  https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line388
 
  There is no end of file chunk.. :)

Fixed, My bad :) Even end of file event hardly made any sense , made it 
End-Of-File received from master. The master closed the event stream


- Anand


---
This is an automatically generated e-mail. To 

Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Vinod Kone

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



src/scheduler/scheduler.cpp (lines 328 - 332)
https://reviews.apache.org/r/37303/#comment149702

can you use the response decoder here?



src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment149689

indent by 4 spaces.



src/scheduler/scheduler.cpp (line 330)
https://reviews.apache.org/r/37303/#comment149690

indent by 4 spaces.



src/scheduler/scheduler.cpp (line 346)
https://reviews.apache.org/r/37303/#comment149691

also print the response.get().status?



src/scheduler/scheduler.cpp (line 436)
https://reviews.apache.org/r/37303/#comment149694

why is this method private but most others are protected?



src/scheduler/scheduler.cpp (line 447)
https://reviews.apache.org/r/37303/#comment149693

const?



src/scheduler/scheduler.cpp (line 475)
https://reviews.apache.org/r/37303/#comment149685

as discussed, please make this a paramter to Mesos() constructor. add a 
TODO for now and follow up in a different patch.


- Vinod Kone


On Aug. 11, 2015, 12:17 a.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 11, 2015, 12:17 a.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
   src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar
 




Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Anand Mazumdar

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

(Updated Aug. 11, 2015, 10:18 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Updated to using RecordIO reader now. Also setting reader to none on receiving 
another subscribe event.


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


Repository: mesos


Description
---

This changes moves scheduler library to http. The change to remove auth + 
install,receive handlers are in other reviews.


Diffs (updated)
-

  src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
  src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
  src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 

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


Testing
---

make check


Thanks,

Anand Mazumdar



Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Anand Mazumdar


 On Aug. 11, 2015, 6:55 p.m., Vinod Kone wrote:
  src/scheduler/scheduler.cpp, line 329
  https://reviews.apache.org/r/37303/diff/2/?file=1036937#file1036937line329
 
  indent by 4 spaces.

Not used now. Fixed the other one.


- Anand


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


On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 11, 2015, 10:18 p.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
   src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar
 




Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Ben Mahler

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


Thanks Anand, mostly thinking we can clean up the read logic if we have a 
struct to capture the reader / decoder.


src/common/http.hpp (lines 51 - 53)
https://reviews.apache.org/r/37303/#comment149809

Can't we just have support for stringifying these?

e.g. stringify(ContentType::PROTOBUF) == APPLICATION_PROTOBUF

Or is there something I'm missing?



src/common/http.hpp (lines 80 - 81)
https://reviews.apache.org/r/37303/#comment149808

value.get() need to be called only if value.isSome()



src/scheduler/scheduler.cpp (line 111)
https://reviews.apache.org/r/37303/#comment149810

This is just for testing right? Might be nice to expose as a separate 
constructor with a note that it's for testing.



src/scheduler/scheduler.cpp (line 117)
https://reviews.apache.org/r/37303/#comment149812

Hm.. can we avoid the 'headers' variable? Seems clearer to keep it local to 
an individual request for now.



src/scheduler/scheduler.cpp (lines 219 - 221)
https://reviews.apache.org/r/37303/#comment149817

This is required, so how about just saying:

```
// Each subscription requires a new connection.
disconnect();
```



src/scheduler/scheduler.cpp (lines 222 - 223)
https://reviews.apache.org/r/37303/#comment149816

Can we call this `disconnect`? Any reason to not clear the reader within 
the function rather than in the caller?



src/scheduler/scheduler.cpp (line 323)
https://reviews.apache.org/r/37303/#comment149824

Need to deal with isFailed case before you call .get() as well.



src/scheduler/scheduler.cpp (line 325)
https://reviews.apache.org/r/37303/#comment149820

Looks like you don't need this, how about:

if (call.type() == Call::SUBSCRIBE 
response.get().status == process::http::statuses[200]) {
  ...   
} else if (response.get().status == process::http::statuses[202]) {
  ...
} else {
  error!
}



src/scheduler/scheduler.cpp (line 328)
https://reviews.apache.org/r/37303/#comment149818

Can you flip this comparison?



src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment149819

CHECK_EQ?



src/scheduler/scheduler.cpp (lines 332 - 338)
https://reviews.apache.org/r/37303/#comment149822

Seems like having a higher level struct, like we did with HttpConnection in 
the master, will help us here as well?

That can provide a 'read' function that returns a FutureEvent. When we 
process the read, we can see if the connection is still the same in order to 
decide whether to continue reading.



src/scheduler/scheduler.cpp (lines 353 - 355)
https://reviews.apache.org/r/37303/#comment149821

How about:

```
Received a '500 Internal Error' for SUBSCRIBE call: Body of request
```

Remember that we don't use periods in logging messages



src/scheduler/scheduler.cpp (line 383)
https://reviews.apache.org/r/37303/#comment149826

Failed to decode the stream of events: ...



src/scheduler/scheduler.cpp (lines 390 - 397)
https://reviews.apache.org/r/37303/#comment149825

Why does master disconnection send an Error event? This can occur if the 
master crashes, fails over, etc. So the scheduler should not see an error for 
this?



src/scheduler/scheduler.cpp (lines 400 - 401)
https://reviews.apache.org/r/37303/#comment149827

s/chunk/event/

How about:

```
Failed to de-serialize event sent by master: ...
```



src/scheduler/scheduler.cpp (line 435)
https://reviews.apache.org/r/37303/#comment149813

Can we call this 'disconnect'?



src/scheduler/scheduler.cpp (line 437)
https://reviews.apache.org/r/37303/#comment149814

space here :)



src/scheduler/scheduler.cpp (line 439)
https://reviews.apache.org/r/37303/#comment149815

Can we avoid saying reader in these messages? Let's just say that the 
connection was already closed, since the users of this library don't care about 
the implementation detail of there being a Reader.


- Ben Mahler


On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 11, 2015, 10:18 p.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/common/http.cpp 

Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Ben Mahler

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


Sorry for not elaborating on all of these, I added some more explanations here. 
Main thing is cleaning up the read loop and figuring out the callback semantics 
(do we need to revisit 'connected' / 'disconnected'?).


src/common/http.hpp (lines 56 - 57)
https://reviews.apache.org/r/37303/#comment149859

You can `return stream  ...;` in a single statement.



src/common/http.hpp (lines 97 - 99)
https://reviews.apache.org/r/37303/#comment149860

Feel free to just return directly, rather than storing in a local variable.



src/scheduler/scheduler.cpp (line 117)
https://reviews.apache.org/r/37303/#comment149861

Sorry, let me elaborate further. I mentioned not having this because 
headers requires non-local reasoning when reading the request sending code:

```
  // Send a streaming request for Subscribe call.
  response = process::http::streaming::post(
  master.get(),
  api/v1/scheduler,
  headers, // non-local
  body,
  stringify(contentType));
  
vs.

  // Send a streaming request for Subscribe call.
  response = process::http::streaming::post(
  master.get(),
  api/v1/scheduler,
  {{Accept, stringify(contentType)}}, // local
  body,
  stringify(contentType));
```

We've generally found local reasoning makes code easier to read (a.k.a. 
readable). Also, keep in mind that in the future you'll be able to send a 
Request directly, so it would become:

```
Request request;
...
request.headers[Accept] = stringify(contentType);

response = process::http::streaming::request(request);
```

The other concern is that headers is not necessarily going to remain 
constant like this in the future (e.g. if we add the notion of a session 
header). Make sense?



src/scheduler/scheduler.cpp (line 323)
https://reviews.apache.org/r/37303/#comment149863

remove the space here



src/scheduler/scheduler.cpp (line 329)
https://reviews.apache.org/r/37303/#comment149862

The reason I ask to flip this is we've generally not used yoda style 
comparisons, e.g.

```
size()  1

rather than

1  size()
```

Can you do a sweep in the code you've introduced here?



src/scheduler/scheduler.cpp (line 330)
https://reviews.apache.org/r/37303/#comment149864

Flip the comparison here as well. Be sure to do a sweep.



src/scheduler/scheduler.cpp (lines 354 - 362)
https://reviews.apache.org/r/37303/#comment149868

I suggested the struct because passing around the recodio reader 
independently of the pipe reader seems to be not that intuitive, was hoping to 
see them stored together inside an OptionConnection rather than having the 
OptionPipe::Reader and recordio::Reader stored separately. Make more sense?

Also, can we use recordio::Reader to be more explicit about this type?



src/scheduler/scheduler.cpp (lines 387 - 394)
https://reviews.apache.org/r/37303/#comment149865

Ok, let's chat about this what to do here, might need to revisit the 
callbacks here now that we have http. But this shouldn't be an error since it's 
just disconnection. If we call disconnected here though, seems that we need to 
immediately call connected if there is still a master available, otherwise we 
might hang around when we should be retrying.



src/scheduler/scheduler.cpp (lines 388 - 389)
https://reviews.apache.org/r/37303/#comment149866

There is no end of file chunk.. :)


- Ben Mahler


On Aug. 12, 2015, 4:12 a.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 12, 2015, 4:12 a.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar
 




Re: Review Request 37303: Moved scheduler library to http

2015-08-11 Thread Anand Mazumdar


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  Thanks Anand, mostly thinking we can clean up the read logic if we have a 
  struct to capture the reader / decoder.

Isn't it much more simpler here?  It's just a one liner if check to check if 
the reader is reader is not None and != for stale reader check. Here is the 
code snipped I have used:
if (!reader.isSome() || reader.get() != oldReader) {


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 353-355
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line353
 
  How about:
  
  ```
  Received a '500 Internal Error' for SUBSCRIBE call: Body of request
  ```
  
  Remember that we don't use periods in logging messages

Sounds good to me. I suppose we don't use periods to terminate logging messages 
?


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 390-397
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line390
 
  Why does master disconnection send an Error event? This can occur if 
  the master crashes, fails over, etc. So the scheduler should not see an 
  error for this?

How else would you communicate to the scheduler that it needs to subscribe 
again in case of master disconnection/failover ? Feel free to re-open in case I 
missed something


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 328
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line328
 
  Can you flip this comparison?

Why would you want to do that ? process::http::statuses[200] is a constant and 
helps identify bugs like if (a = 5) by keeping the constant check on the left ? 
Seems like I am missing something here


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 111
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line111
 
  This is just for testing right? Might be nice to expose as a separate 
  constructor with a note that it's for testing.

Not quite. I intend to add an constructor to Mesos::Mesos i.e. the wrapper 
exposed class to the user later that would be used for testing. This is just 
the internal implementation class.


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 117
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line117
 
  Hm.. can we avoid the 'headers' variable? Seems clearer to keep it 
  local to an individual request for now.

Why ? This is anyways just the internal implementation class. Since we have to 
do that for every request we make. Why not just save it as a constant member 
variable ?


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 219-221
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line219
 
  This is required, so how about just saying:
  
  ```
  // Each subscription requires a new connection.
  disconnect();
  ```

Sounds good. Done


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/common/http.hpp, lines 80-81
  https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line80
 
  value.get() need to be called only if value.isSome()

Fixed , my bad.


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, lines 222-223
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line222
 
  Can we call this `disconnect`? Any reason to not clear the reader 
  within the function rather than in the caller?

Fixed


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 323
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line323
 
  Need to deal with isFailed case before you call .get() as well.

Fixed


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 329
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line329
 
  CHECK_EQ?

Fixed


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 435
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line435
 
  Can we call this 'disconnect'?

Fixed


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 437
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line437
 
  space here :)

I wish we as a community can get adoption for clang format soon. These things 
should be handled by a tool rather then someone spending time reviewing them :(


 On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
  src/scheduler/scheduler.cpp, line 439
  https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line439
 
  Can we avoid saying reader in these messages? Let's just say that the 
  connection was already closed, since the users of this library don't care 
  about the implementation detail of there being a Reader.

Fixed


 On 

Re: Review Request 37303: Moved scheduler library to http

2015-08-10 Thread Vinod Kone

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


looks like the SchedulerTest.Subscribe test failed?


src/common/http.hpp (line 66)
https://reviews.apache.org/r/37303/#comment149373

// Deserializes a string message into a protobuf message
// based on the HTTP content type



src/common/http.hpp (line 67)
https://reviews.apache.org/r/37303/#comment149378

hmm. this is not as generic as serialize. can't this be templated?

template typename T
TryT deserialize(...);



src/common/http.hpp (line 73)
https://reviews.apache.org/r/37303/#comment149375

this helper seems weird. i would rather have isOK()and isAccepted() helpers.

anyway, this seems to be used only once. do we even need a helper?



src/common/http.cpp (line 86)
https://reviews.apache.org/r/37303/#comment149379

add a default case please, in case someone adds a new supported contentType 
enum.



src/common/http.cpp (line 101)
https://reviews.apache.org/r/37303/#comment149380

ditto. add a default case.



src/scheduler/scheduler.cpp (line 211)
https://reviews.apache.org/r/37303/#comment149382

why new synchronous function (_send()) here instead of directly plopping 
the code here?

we prefer procedural programming style, i.e., we don't introduce new 
methods/functions unless it's necesary.



src/scheduler/scheduler.cpp 
https://reviews.apache.org/r/37303/#comment149383

this should've been in the patch that removed pid related stuff.



src/scheduler/scheduler.cpp (line 296)
https://reviews.apache.org/r/37303/#comment149384

s/call type Subscribe/Subscribe call/



src/scheduler/scheduler.cpp (line 314)
https://reviews.apache.org/r/37303/#comment149386

```
s/handleResponse/_send/
```



src/scheduler/scheduler.cpp (line 315)
https://reviews.apache.org/r/37303/#comment149388

handling error like this loses the context about the call that caused the 
error, no?

i would just do a .onAny() instead of .then() and .onFailed().



src/scheduler/scheduler.cpp (line 320)
https://reviews.apache.org/r/37303/#comment149385

you should make sure subscribe got an OK and everything else got Accepted. 
for that this method needs to take Call as argument.



src/scheduler/scheduler.cpp (line 322)
https://reviews.apache.org/r/37303/#comment149389

ditto. this should include which call/request failed.



src/scheduler/scheduler.cpp (line 334)
https://reviews.apache.org/r/37303/#comment149392

This needs a comment. Why would there be an existing reader? Why is an 
existing reader not closed as soon as a subscribe call is made?



src/scheduler/scheduler.cpp (line 345)
https://reviews.apache.org/r/37303/#comment149390

are all other response types (non PIP) ok to receive?



src/scheduler/scheduler.cpp (lines 358 - 359)
https://reviews.apache.org/r/37303/#comment149393

Just say that Events stream filed and include the failure message.



src/scheduler/scheduler.cpp (line 375)
https://reviews.apache.org/r/37303/#comment149394

ditto. please don't create new functions unless there is re-use.



src/scheduler/scheduler.cpp (line 381)
https://reviews.apache.org/r/37303/#comment149396

When does this happen? Add a comment.

Also, sending a subscribe with master.. with every error event seems a 
bit redundat? Can we just add documentation to the ERROR event in the protobuf 
saying so? Or are there events where we dont want schedulers to oversubscribe?



src/scheduler/scheduler.cpp (lines 392 - 393)
https://reviews.apache.org/r/37303/#comment149397

When does this happen?



src/scheduler/scheduler.cpp (lines 406 - 407)
https://reviews.apache.org/r/37303/#comment149400

include the error please.



src/scheduler/scheduler.cpp (line 423)
https://reviews.apache.org/r/37303/#comment149401

make this a LOG(WARNING)



src/scheduler/scheduler.cpp (lines 483 - 490)
https://reviews.apache.org/r/37303/#comment149387

```
process = new MesosProcess(
arg1,
arg2,
arg3,
arg4,
arg5);
```


- Vinod Kone


On Aug. 10, 2015, 4:43 p.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 10, 2015, 4:43 p.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   

Re: Review Request 37303: Moved scheduler library to http

2015-08-10 Thread Anand Mazumdar

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

(Updated Aug. 11, 2015, 12:17 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Vinod's review comments.


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


Repository: mesos


Description
---

This changes moves scheduler library to http. The change to remove auth + 
install,receive handlers are in other reviews.


Diffs (updated)
-

  src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
  src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
  src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 

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


Testing
---

make check


Thanks,

Anand Mazumdar



Re: Review Request 37303: Moved scheduler library to http

2015-08-10 Thread Anand Mazumdar


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/common/http.hpp, line 73
  https://reviews.apache.org/r/37303/diff/1/?file=1036466#file1036466line73
 
  this helper seems weird. i would rather have isOK()and isAccepted() 
  helpers.
  
  anyway, this seems to be used only once. do we even need a helper?

Removed method.


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/common/http.cpp, line 86
  https://reviews.apache.org/r/37303/diff/1/?file=1036467#file1036467line86
 
  add a default case please, in case someone adds a new supported 
  contentType enum.

Why ? I want the compilation to fail when a user adds a new supported 
contentType enum ( -WSwitch )


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/common/http.cpp, line 101
  https://reviews.apache.org/r/37303/diff/1/?file=1036467#file1036467line101
 
  ditto. add a default case.

Same as above.


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/scheduler/scheduler.cpp, line 354
  https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line354
 
  you should make sure subscribe got an OK and everything else got 
  Accepted. for that this method needs to take Call as argument.

Logging an error now for such cases.


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/scheduler/scheduler.cpp, lines 426-427
  https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line426
 
  When does this happen?

This can happen when there might be some corruption of data across the wire, or 
the server sends a in-complete protobuf/json response. Added the error from 
decoder to the string too.


 On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
  src/scheduler/scheduler.cpp, line 415
  https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line415
 
  When does this happen? Add a comment.
  
  Also, sending a subscribe with master.. with every error event seems 
  a bit redundat? Can we just add documentation to the ERROR event in the 
  protobuf saying so? Or are there events where we dont want schedulers to 
  oversubscribe?

Added.


- Anand


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


On Aug. 11, 2015, 12:17 a.m., Anand Mazumdar wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37303/
 ---
 
 (Updated Aug. 11, 2015, 12:17 a.m.)
 
 
 Review request for mesos, Ben Mahler and Vinod Kone.
 
 
 Bugs: MESOS-2552
 https://issues.apache.org/jira/browse/MESOS-2552
 
 
 Repository: mesos
 
 
 Description
 ---
 
 This changes moves scheduler library to http. The change to remove auth + 
 install,receive handlers are in other reviews.
 
 
 Diffs
 -
 
   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
   src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
   src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830 
 
 Diff: https://reviews.apache.org/r/37303/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Anand Mazumdar