date:20150811

Re: Review Request 37303: Moved scheduler library to http

2015-08-11 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 Option rather than having 
> > the Option 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
> > 
> >
> > 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
> > 
> >
> > There is no "end of file chunk".. :)

Fixed, My bad :) Even end of file event hardly made any sense

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. 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 37382: Introduced provisioner Backend interface.

2015-08-11 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37382]

All tests passed.

- Mesos ReviewBot


On Aug. 12, 2015, 5:49 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37382/
> ---
> 
> (Updated Aug. 12, 2015, 5:49 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2968
> https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Backend interface is made generic so it can be used by different 
> provisioners. See 
> [MESOS-2968](https://issues.apache.org/jira/browse/MESOS-2968?focusedCommentId=14652859&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14652859)
>  for details.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 07502f0f5523b972888ceab9cf3af309c8441d7f 
>   src/slave/containerizer/provisioners/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backend.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37382/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 37382: Introduced provisioner Backend interface.

2015-08-11 Thread Jiang Yan Xu


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

Review request for mesos, Lily Chen, Ian Downes, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

The Backend interface is made generic so it can be used by different 
provisioners. See 
[MESOS-2968](https://issues.apache.org/jira/browse/MESOS-2968?focusedCommentId=14652859&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14652859)
 for details.


Diffs
-

  src/Makefile.am 07502f0f5523b972888ceab9cf3af309c8441d7f 
  src/slave/containerizer/provisioners/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backend.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

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)

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

src/common/http.hpp (lines 97 - 99)

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

src/scheduler/scheduler.cpp (line 117)

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)

remove the space here

src/scheduler/scheduler.cpp (line 329)

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)

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

src/scheduler/scheduler.cpp (lines 354 - 362)

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 Option rather than having the 
Option 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)

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)

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


---
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.


Changes
---

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-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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > CHECK_EQ?

Fixed

> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 435
> > 
> >
> > Can we call this 'disconnect'?

Fixed

> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 437
> > 
> >
> > 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
> > 
> >
> > Can we avoid saying "reader" in these me

Re: Review Request 37378: Updated http api tests to use V1 Protobufs

2015-08-11 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37082, 37192, 37378]

All tests passed.

- Mesos ReviewBot


On Aug. 12, 2015, 12:14 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37378/
> ---
> 
> (Updated Aug. 12, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated to use v1 protobufs for http api tests. Added a default conversion 
> function for FrameworkInfo in devolve()
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.hpp b9a854a5e2454a4c3f2b85032b928b2cbc0b000c 
>   src/internal/devolve.cpp be74a26e9a194a8d969c667fdc084964939dfdb7 
>   src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf 
> 
> Diff: https://reviews.apache.org/r/37378/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 37378: Updated http api tests to use V1 Protobufs

2015-08-11 Thread Vinod Kone


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

Ship it!


Ship It!

- Vinod Kone


On Aug. 12, 2015, 12:14 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37378/
> ---
> 
> (Updated Aug. 12, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated to use v1 protobufs for http api tests. Added a default conversion 
> function for FrameworkInfo in devolve()
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.hpp b9a854a5e2454a4c3f2b85032b928b2cbc0b000c 
>   src/internal/devolve.cpp be74a26e9a194a8d969c667fdc084964939dfdb7 
>   src/tests/http_api_tests.cpp 23214dfc770d9b18c85ddbdaf35b85e59eeb8acf 
> 
> Diff: https://reviews.apache.org/r/37378/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35983: Added /unreserve HTTP endpoint to the master.

2015-08-11 Thread Michael Park


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

(Updated Aug. 12, 2015, 2:46 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
Van Remoortere, and Vinod Kone.


Changes
---

Discarded [r36987](https://reviews.apache.org/r/36987/) and rebased accordingly.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37 
  src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96 
  src/master/master.cpp 398203d9367f85340166e66ecc34b9a33dd81048 
  src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5 
  src/tests/master_validation_tests.cpp 
3513bca6fd6773f712d1a647b1757766dc34f948 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 35702: Added /reserve HTTP endpoint to the master.

2015-08-11 Thread Michael Park

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

(Updated Aug. 12, 2015, 2:44 a.m.)

Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris
Van Remoortere, and Vinod Kone.

Changes
---

Discarded [r36987](https://reviews.apache.org/r/36987/) and rebased accordingly.

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

Repository: mesos

Description
---

This involved a lot more challenges than I anticipated, I've captured the
various approaches and limitations and deal-breakers of those approaches here:
[Master Endpoint Implementation
Challenges](https://docs.google.com/document/d/1cwVz4aKiCYP9Y4MOwHYZkyaiuEv7fArCye-vPvB2lAI/edit#)

Key points:

* This is a stop-gap solution until we shift the offer creation/management
logic from the master to the allocator.
* `updateAvailable` and `updateSlave` are kept separate because
(1) `updateAvailable` is allowed to fail whereas `updateSlave` must not.
(2) `updateAvailable` returns a `Future` whereas `updateSlave` does not.
(3) `updateAvailable` never leaves the allocator in an over-allocated state
and must not, whereas `updateSlave` does, and can.
* The algorithm:
* Initially, the master pessimistically assume that what seems like
"available" resources will be gone.
This is due to the race between the allocator scheduling an `allocate`
call to itself vs master's
`allocator->updateAvailable` invocation.
As such, we first try to satisfy the request only with the offered
resources.
* We greedily rescind one offer at a time until we've rescinded
sufficiently many offers.
IMPORTANT: We perform `recoverResources(..., Filters())` which has a
default `refuse_sec` of 5 seconds,
rather than `recoverResources(..., None())` so that we can virtually
always win the race against `allocate`.
In the rare case that we do lose, no disaster occurs. We simply fail to
satisfy the request.
* If we still don't have enough resources after resciding all offers, be
semi-optimistic and forward the
request to the allocator since there may be available resources to
satisfy the request.
* If the allocator returns a failure, report the error to the user with
`Conflict`.

This approach is clearly not ideal, since we would prefer to rescind as little
offers as possible.

Diffs (updated)
-

src/master/http.cpp 7c650555ed15d310ad28e68239cbd56580fbae37
src/master/master.hpp bb7c8e9e05be829a6b9aa3100a714b2359854d96
src/master/master.cpp 398203d9367f85340166e66ecc34b9a33dd81048
src/master/validation.hpp 43b8d84556e7f0a891dddf6185bbce7ca50b360a
src/master/validation.cpp ffb7bf07b8a40d6e14f922eabcf46045462498b5

1 2 >

1 - 100 of 115 matches

Mail list logo