Re: Review Request 49223: Enhanced Value parsing.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49223]

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

- Mesos ReviewBot


On June 27, 2016, 3:22 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49223/
> ---
> 
> (Updated June 27, 2016, 3:22 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enhanced Value parsing:
> 
> 1. Did not support `[1-2, [3-4]]` as Ranges; it should be `[1-2, 3-4]`.
> 2. Did not support `{a{b, c}d}` as Set; it should be `{ab, cd}`
> 3. Add check for Text against `[a-zA-Z0-9_/.-]`
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
>   src/tests/attributes_tests.cpp cb71be5ecead322d90943146f54f8a0c915eba1c 
>   src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 
> 
> Diff: https://reviews.apache.org/r/49223/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 32058: Added protobuf-JSON validation

[Jay Guo]

> On April 16, 2015, 2:46 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 573
> > 
> >
> > Just as Vinod commented on the JIRA, we might want  to do that 
> > differently - e.g. by adding a factory.
> 
> Akanksha Agrawal wrote:
> I did not understand how this should be done. Can you elaborate it a bit?
> 
> Jay Guo wrote:
> I think this patch is very outdated, could you rebase first? I failed to 
> find this class...

And I think a factory method to instantiate protobuf could be something like:
```
Try createProtobuf(message)
{
  if message is valid:
return Protobuf(message);
  else:
return Error("...");
  end
  
  UNREACHABLE();
}
```
and put Protobuf constructor `Protobuf(message)` in private.


- Jay


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


On March 16, 2015, 8:02 p.m., Akanksha Agrawal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32058/
> ---
> 
> (Updated March 16, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-1194
> https://issues.apache.org/jira/browse/MESOS-1194
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf-JSON validation
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 2c020d98b94995bce862ac3b9c173dd64ed1198d 
> 
> Diff: https://reviews.apache.org/r/32058/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akanksha Agrawal
> 
>

Re: Review Request 32058: Added protobuf-JSON validation

[Jay Guo]

> On April 16, 2015, 2:46 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 573
> > 
> >
> > Just as Vinod commented on the JIRA, we might want  to do that 
> > differently - e.g. by adding a factory.
> 
> Akanksha Agrawal wrote:
> I did not understand how this should be done. Can you elaborate it a bit?

I think this patch is very outdated, could you rebase first? I failed to find 
this class...


- Jay


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


On March 16, 2015, 8:02 p.m., Akanksha Agrawal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32058/
> ---
> 
> (Updated March 16, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-1194
> https://issues.apache.org/jira/browse/MESOS-1194
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf-JSON validation
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 2c020d98b94995bce862ac3b9c173dd64ed1198d 
> 
> Diff: https://reviews.apache.org/r/32058/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akanksha Agrawal
> 
>

Re: Review Request 49240: Update HttpDockerExecutor with v1 API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49240]

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

- Mesos ReviewBot


On June 27, 2016, 2:29 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49240/
> ---
> 
> (Updated June 27, 2016, 2:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Qian Zhang, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5227
> https://issues.apache.org/jira/browse/MESOS-5227
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is part of the MESOS-5227:
> Implement HTTP Docker Executor that uses the Executor Library.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 88b7fc4c36ed3974ac6b103a29e1d975619f0c69 
>   src/internal/devolve.hpp 4a6ae681d37b3405ee81c4a58388b8d501743ebf 
>   src/internal/devolve.cpp cecb22e49614c6ee47489eead1d3161e033a53ef 
> 
> Diff: https://reviews.apache.org/r/49240/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 49228: Fixed wrong stopped time in webui for live tasks.

[Vinod Kone]

> On June 25, 2016, 3:26 p.m., Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js, line 162
> > 
> >
> > Probably `task.state` could be used here.

yup. just use `task.state` here


- Vinod


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


On June 25, 2016, 3:24 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49228/
> ---
> 
> (Updated June 25, 2016, 3:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, haosdent huang, Ross Allen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only set the terminal time if the last state is terminal.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> be3fa1f1fba9eda7ebcc9bf33142fe4307dd8d90 
> 
> Diff: https://reviews.apache.org/r/49228/diff/
> 
> 
> Testing
> ---
> 
> Generate orpahne tasks that is alive. It should have not set stopped time.
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 48910: Group frameworks by state in the webui.

[Vinod Kone]

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


Fix it, then Ship it!





src/webui/master/static/frameworks.html (line 29)


should be filtered by `disconnected` boolean.

a framework in master has two tags associated with it, `active` and 
`connected`. so the following 4 states are theoretically possible:

connected and active
connected but inactive (not currently possible but maybe in the future)
disconnected and active (doesn't make sense)
disconnected and inactive



src/webui/master/static/frameworks.html (line 87)


s/active/connected/


- Vinod Kone


On June 25, 2016, 3:48 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48910/
> ---
> 
> (Updated June 25, 2016, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Deshna Jain, haosdent huang, Ross 
> Allen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2145
> https://issues.apache.org/jira/browse/MESOS-2145
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Create 3 groups in fameworks listing for connected,
> disconnected and completed frameworks.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/frameworks.html 
> cfb6f4efc259419e4cb697332c8d6f2839e96970 
> 
> Diff: https://reviews.apache.org/r/48910/diff/
> 
> 
> Testing
> ---
> 
> ![screenshot](https://issues.apache.org/jira/secure/attachment/12813382/frameworks_alive.png)
> 
> Run framework (e.g. Marathon) wait until it appear on `#/frameworks`. Stop it 
> (no graceful shutdown with deregistration). It should be highlighted with 
> next page update.
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 49136: Add Framework protobuf message.

[Vinod Kone]

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




include/mesos/master/master.proto (lines 307 - 309)


make these optional.



include/mesos/master/master.proto (line 309)


make this optional



include/mesos/master/master.proto (lines 311 - 315)


I would order these so:

```
repeated TaskInfo pending_tasks  = 7;
repeated Task tasks = 8;
repeated Task completed_tasks = 9;

repeated Executor executors = 10;

repeated Offer offers = 11;
```



include/mesos/master/master.proto (line 317)


s/used/allocated/


- Vinod Kone


On June 26, 2016, 1:53 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49136/
> ---
> 
> (Updated June 26, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: mesos-5492
> https://issues.apache.org/jira/browse/mesos-5492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add 'Framework' protobuf message to master/master.proto &
> v1/master/master.proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
> 
> Diff: https://reviews.apache.org/r/49136/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 49157: Added missing comments for operator API protos.

[Vinod Kone]

> On June 24, 2016, 9:25 p.m., Anand Mazumdar wrote:
> > include/mesos/master/master.proto, line 307
> > 
> >
> > hmmm.. we should consider doing a sweep to kill:
> > 
> > `The response for 'Call::GET_X`.
> > 
> > or introduce it for every Response type. Currently, some of them have 
> > it and some don't. Also, it's pretty self-explanatory that `GetX` message 
> > refers to the `Response` for `GET_X` call, no?
> 
> haosdent huang wrote:
> I think it would better to add it, because
> 
> ```
>   message GetFileContents {
> repeated bytes data = 1;
>   }
> ```
> 
> is the response for `Call::READ_FILE`.
> 
> haosdent huang wrote:
> I add the missing `The response for 'Call::GET_X`, do you think we should 
> add them only when necessary?
> 
> Anand Mazumdar wrote:
> Yeah, I was thinking of adding these when we _actually_ need them to 
> resolve ambiguities e.g., if the response message is different from the call 
> name etc. 
> 
> It's fine to add it here for consistency. We can always do a sweep in 
> another patch to remove them if we feel the need.

I think it was an oversight to have inconsistent Call type and Response type. 
Is this the only one? s/GET_FILE_CONTENTS/READ_FILE/


- Vinod


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


On June 25, 2016, 6:44 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49157/
> ---
> 
> (Updated June 25, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta, Anand Mazumdar, zhou xing, Jay 
> Guo, Shuai Lin, and Vinod Kone.
> 
> 
> Bugs: MESOS-5695
> https://issues.apache.org/jira/browse/MESOS-5695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add missing comments for `GET_ROLES`, `GET_WEIGHTS`, `SUBSCRIBE`,
> `CREATE_VOLUMES`, `DESTROY_VOLUMES`, `SET_QUOTA` in operator API
> protos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
> 
> Diff: https://reviews.apache.org/r/49157/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49015: Implemented GET_CONTAINERS Call in v1 Agent API.

[Vinod Kone]

> On June 21, 2016, 11:10 p.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 660
> > 
> >
> > hmm. looks like this is not reusing any code from `_containers()`. can 
> > you extract the common code from `_containers` (e.g., authz) into 
> > `__containers()` and re-use that here. similar to what we did with 
> > `getTasks()`.
> 
> Jay Guo wrote:
> I thought of reusing common logic however returned value (JSON for 
> `_containers` and v1Response for `getContainers`) is assembled along with the 
> workflow. Unless we define a common data structure and evolve it according to 
> request type (v1 call or containers endpoint). Do you think it makes sense to 
> jsonify v1response for `_containers`?
> 
> Jay Guo wrote:
> And I think it's better to migrate containers endpoint to leverage 
> jsonify first and see how we could reuse the logic. I'll investigate it.
> 
> Vinod Kone wrote:
> how about evolving the JSON object returned by `containers()` into the 
> `Containers` proto in evolve.cpp? just like what we did with Flags, Version 
> etc. This endpoint is not that expensive because it only lists live 
> containers on an agent; so evolving from JSON is ok. what do you think?
> 
> Jay Guo wrote:
> That could be done. Although if we are eventually deprecating pre-v1 
> APIs, I think evolving json to protobuf will look strange if the 
> implementation stands alone. Of course we could still do it for now and 
> refactor it again when we delete old `/containers`.

yes, we can delete later.


- Vinod


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


On June 22, 2016, 8:02 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49015/
> ---
> 
> (Updated June 22, 2016, 8:02 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5518
> https://issues.apache.org/jira/browse/MESOS-5518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented GET_CONTAINERS Call in v1 Agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 8f5267367857f45dc5c9ab064b183c9c8f1703e9 
>   include/mesos/v1/agent/agent.proto 91fb55db004733c3c5bb42acc32f1afedbc208bb 
>   src/slave/http.cpp 22b5930a7af336a749af4b6b50254f60433ecea5 
>   src/slave/slave.hpp 58ff2bfac6918d989ab36b67cf6ba2f3657c8356 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/49015/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04 & OSX
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 49015: Implemented GET_CONTAINERS Call in v1 Agent API.

[Vinod Kone]

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




src/slave/http.cpp (line 658)


move this next to `Slave::Http::Containers` method.


- Vinod Kone


On June 22, 2016, 8:02 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49015/
> ---
> 
> (Updated June 22, 2016, 8:02 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5518
> https://issues.apache.org/jira/browse/MESOS-5518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented GET_CONTAINERS Call in v1 Agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 8f5267367857f45dc5c9ab064b183c9c8f1703e9 
>   include/mesos/v1/agent/agent.proto 91fb55db004733c3c5bb42acc32f1afedbc208bb 
>   src/slave/http.cpp 22b5930a7af336a749af4b6b50254f60433ecea5 
>   src/slave/slave.hpp 58ff2bfac6918d989ab36b67cf6ba2f3657c8356 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/49015/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04 & OSX
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 49223: Enhanced Value parsing.

[Klaus Ma]

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

(Updated June 27, 2016, 11:22 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

typo


Repository: mesos


Description (updated)
---

Enhanced Value parsing:

1. Did not support `[1-2, [3-4]]` as Ranges; it should be `[1-2, 3-4]`.
2. Did not support `{a{b, c}d}` as Set; it should be `{ab, cd}`
3. Add check for Text against `[a-zA-Z0-9_/.-]`


Diffs
-

  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/tests/attributes_tests.cpp cb71be5ecead322d90943146f54f8a0c915eba1c 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49223: Enhanced Value parsing.

[Klaus Ma]

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

(Updated June 27, 2016, 11:18 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Enhance UT cases.


Summary (updated)
-

Enhanced Value parsing.


Repository: mesos


Description (updated)
---

Enhanced Value parsing:

1. Did not support `[1-2, [3-4]]` as Ranges; it should be `[1-2, 3-4]`.
2. Did not support `{a{b, c}d}` as Set; it should be `{ab, cd}`
3. Add check for Test agains `[a-zA-Z0-9_/.-]`


Diffs (updated)
-

  src/common/values.cpp 587cb68551d438621e215953e89818b623b7f71b 
  src/tests/attributes_tests.cpp cb71be5ecead322d90943146f54f8a0c915eba1c 
  src/tests/values_tests.cpp 929861549e3155c33966896f817f9bf9e6d14354 

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


Testing (updated)
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49214: Added `FileInfo` protobuf for describing a File.

[zhou xing]

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


Ship it!




Ship it, I'm adding similar GetFileContents into mesos.proto, will submit today

- zhou xing


On 六月 24, 2016, 9:13 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49214/
> ---
> 
> (Updated 六月 24, 2016, 9:13 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta, zhou xing, and Vinod Kone.
> 
> 
> Bugs: MESOS-5514
> https://issues.apache.org/jira/browse/MESOS-5514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change would be needed later for implementing the `LIST_FILES`
> call for the Master/Agent v1 API.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8a417f91ca6771b971cf5ee50351171412163f7a 
>   include/mesos/v1/mesos.proto 3bc65e7fb1b34651bf025711023f9e219928040a 
> 
> Diff: https://reviews.apache.org/r/49214/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 49240: Update HttpDockerExecutor with v1 API.

[Yong Tang]

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

Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Qian Zhang, and 
Vinod Kone.


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


Repository: mesos


Description
---

This patch is part of the MESOS-5227:
Implement HTTP Docker Executor that uses the Executor Library.


Diffs
-

  src/docker/executor.cpp 88b7fc4c36ed3974ac6b103a29e1d975619f0c69 
  src/internal/devolve.hpp 4a6ae681d37b3405ee81c4a58388b8d501743ebf 
  src/internal/devolve.cpp cecb22e49614c6ee47489eead1d3161e033a53ef 

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


Testing
---

make check on Ubuntu 14.04


Thanks,

Yong Tang

Re: Review Request 49226: Implemented UNRESERVE_RESOURCES Call in v1 master API.

[Jay Guo]

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




src/tests/api_tests.cpp (lines 93 - 99)


Move this to the first patch in this review chain and maybe generalize it 
and add to `tests/mesos.hpp`


- Jay Guo


On June 25, 2016, 6:54 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49226/
> ---
> 
> (Updated June 25, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5500
> https://issues.apache.org/jira/browse/MESOS-5500
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented UNRESERVE_RESOURCES Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 70f084b84db90fde20c05d2354be190f28e72996 
>   src/master/master.hpp e983d1ba6ebcdaf2ace419201659e53edaa2a0aa 
>   src/tests/api_tests.cpp 7f16f43c3968cd56cf93951489079032093beaeb 
> 
> Diff: https://reviews.apache.org/r/49226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49239: Added logging when Offer::Operation::Launch has no tasks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49239]

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

- Mesos ReviewBot


On June 27, 2016, 12:43 a.m., Jose Guilherme Vanz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49239/
> ---
> 
> (Updated June 27, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-5565
> https://issues.apache.org/jira/browse/MESOS-5565
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master.cpp has been updated adding a warning message when some
> offer is accepted by the framework but any task is defined to be
> executed
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp d89c049326358bad509bb1455c81eb11610eeeb8 
> 
> Diff: https://reviews.apache.org/r/49239/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jose Guilherme Vanz
> 
>

Re: Review Request 49015: Implemented GET_CONTAINERS Call in v1 Agent API.

[Jay Guo]

> On June 21, 2016, 11:10 p.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 660
> > 
> >
> > hmm. looks like this is not reusing any code from `_containers()`. can 
> > you extract the common code from `_containers` (e.g., authz) into 
> > `__containers()` and re-use that here. similar to what we did with 
> > `getTasks()`.
> 
> Jay Guo wrote:
> I thought of reusing common logic however returned value (JSON for 
> `_containers` and v1Response for `getContainers`) is assembled along with the 
> workflow. Unless we define a common data structure and evolve it according to 
> request type (v1 call or containers endpoint). Do you think it makes sense to 
> jsonify v1response for `_containers`?
> 
> Jay Guo wrote:
> And I think it's better to migrate containers endpoint to leverage 
> jsonify first and see how we could reuse the logic. I'll investigate it.
> 
> Vinod Kone wrote:
> how about evolving the JSON object returned by `containers()` into the 
> `Containers` proto in evolve.cpp? just like what we did with Flags, Version 
> etc. This endpoint is not that expensive because it only lists live 
> containers on an agent; so evolving from JSON is ok. what do you think?

That could be done. Although if we are eventually deprecating pre-v1 APIs, I 
think evolving json to protobuf will look strange if the implementation stands 
alone. Of course we could still do it for now and refactor it again when we 
delete old `/containers`.


- Jay


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


On June 22, 2016, 8:02 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49015/
> ---
> 
> (Updated June 22, 2016, 8:02 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5518
> https://issues.apache.org/jira/browse/MESOS-5518
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented GET_CONTAINERS Call in v1 Agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 8f5267367857f45dc5c9ab064b183c9c8f1703e9 
>   include/mesos/v1/agent/agent.proto 91fb55db004733c3c5bb42acc32f1afedbc208bb 
>   src/slave/http.cpp 22b5930a7af336a749af4b6b50254f60433ecea5 
>   src/slave/slave.hpp 58ff2bfac6918d989ab36b67cf6ba2f3657c8356 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/49015/diff/
> 
> 
> Testing
> ---
> 
> make check on Ubuntu 14.04 & OSX
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 49140: Added startsWith/endsWith to support char.

[Klaus Ma]

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



Sync up with mycpark offline, I'll also update the string version of 
`startsWith/endsWith` to improve the performance.

- Klaus Ma


On June 26, 2016, 5:46 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49140/
> ---
> 
> (Updated June 26, 2016, 5:46 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-5692
> https://issues.apache.org/jira/browse/MESOS-5692
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added startsWith/endsWith to support char.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 
> 
> Diff: https://reviews.apache.org/r/49140/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 49239: Added logging when Offer::Operation::Launch has no tasks.

[Jose Guilherme Vanz]

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

(Updated June 26, 2016, 9:43 p.m.)


Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

The master.cpp has been updated adding a warning message when some
offer is accepted by the framework but any task is defined to be
executed


Diffs
-

  src/master/master.cpp d89c049326358bad509bb1455c81eb11610eeeb8 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Review Request 49239: Added logging when Offer::Operation::Launch has no tasks.

[Jose Guilherme Vanz]

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

Review request for mesos.


Repository: mesos


Description
---

The master.cpp has been updated adding a warning message when some
offer is accepted by the framework but any task is defined to be
executed


Diffs
-

  src/master/master.cpp d89c049326358bad509bb1455c81eb11610eeeb8 

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


Testing
---


Thanks,

Jose Guilherme Vanz

Re: Review Request 49225: Implemented RESERVE_RESOURCES Call in v1 master API.

[Anand Mazumdar]

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




src/master/http.cpp (lines 1505 - 1506)


Do we need this explicit check here? I can understand why we 
introduced/need it in `GET_LEADER` but why here? Bad Copy/Paste?



src/master/http.cpp (line 1508)


Capture by constant reference since it's an alias.



src/master/http.cpp (line 1515)


Capture by constant reference since it's an alias.



src/master/http.cpp (line 1675)


Pass by reference please.



src/master/master.hpp (line 1310)


`const Resources&`



src/master/master.hpp (line 1447)


s/acceptType/contentType



src/tests/api_tests.cpp (line 51)


Alphabetical please. Move it to L49



src/tests/api_tests.cpp (line 504)


Usually we avoid such one trivial liner comments that hardly serve any 
purpose/help improve readability of the code.

Can you instead modify this comment with a brief overview of what the test 
_intends_ to do? See similar tests in other files for inspiration.



src/tests/api_tests.cpp (line 523)


Where is `createFrameworkInfo` defined?



src/tests/api_tests.cpp (line 545)


It would be good to use the operator `->` here. Just `offers->size()` 
should work. Here and in other places in the test.



src/tests/api_tests.cpp (line 563)


Nit: s/mesos::internal/internal

Here and in all the other places in this test please.



src/tests/api_tests.cpp (lines 567 - 568)


Do we need the explicit `static_cast` here? Why doesn't `CopyFrom()` work 
directly?



src/tests/api_tests.cpp (line 572)


Kill this comment



src/tests/api_tests.cpp (line 580)


Kill extra new line here


- Anand Mazumdar


On June 25, 2016, 6:52 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49225/
> ---
> 
> (Updated June 25, 2016, 6:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5499
> https://issues.apache.org/jira/browse/MESOS-5499
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented RESERVE_RESOURCES Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 70f084b84db90fde20c05d2354be190f28e72996 
>   src/master/master.hpp e983d1ba6ebcdaf2ace419201659e53edaa2a0aa 
>   src/tests/api_tests.cpp 7f16f43c3968cd56cf93951489079032093beaeb 
> 
> Diff: https://reviews.apache.org/r/49225/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49206: Implemented GET_QUOTA Call in v1 master API.

[Anand Mazumdar]

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



Looks good minus minor comments on style/indent.


include/mesos/master/master.proto (line 334)


s/quota info/cluster's configured quotas.

Ditto for the versioned one.



src/master/quota_handler.cpp (line 222)


Indent off by 1 for `stringify`.



src/master/quota_handler.cpp (line 268)


hmm .. can we move `__status` to inside the lambda itself rather than it 
being a separate function?

- We follow a procedural programming style and have continuation functions 
only when something is waiting on a different execution context.
- In one off cases, we move things out of a lambda when it's too long and 
might impact readability. (e.g., `__set` in this file)

Neithing of the above 2 seem to hold good here.



src/tests/api_tests.cpp (line 917)


Missing backticks around set quota.



src/tests/api_tests.cpp (line 943)


Fix the indent here. (2 spaces)

Add a new line before L943



src/tests/api_tests.cpp (line 944)


Move this comment out of the block and also add backticks.



src/tests/api_tests.cpp (line 948)


Do this after L923 since it's common to both blocks.



src/tests/api_tests.cpp (line 958)


Fix the indent to be right below `quotaResources`.


- Anand Mazumdar


On June 25, 2016, 9:03 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49206/
> ---
> 
> (Updated June 25, 2016, 9:03 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5508
> https://issues.apache.org/jira/browse/MESOS-5508
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented GET_QUOTA Call in v1 master API.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
>   src/master/master.hpp e983d1ba6ebcdaf2ace419201659e53edaa2a0aa 
>   src/master/quota_handler.cpp 4fa7533cb6a109fd060050c7814de440bd34c8ed 
>   src/tests/api_tests.cpp 7f16f43c3968cd56cf93951489079032093beaeb 
> 
> Diff: https://reviews.apache.org/r/49206/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04 :
> 
> sudo GTEST_FILTER="*MasterAPITest.GetQuota*" make -j4 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 49157: Added missing comments for operator API protos.

[Anand Mazumdar]

> On June 24, 2016, 9:25 p.m., Anand Mazumdar wrote:
> > include/mesos/master/master.proto, line 307
> > 
> >
> > hmmm.. we should consider doing a sweep to kill:
> > 
> > `The response for 'Call::GET_X`.
> > 
> > or introduce it for every Response type. Currently, some of them have 
> > it and some don't. Also, it's pretty self-explanatory that `GetX` message 
> > refers to the `Response` for `GET_X` call, no?
> 
> haosdent huang wrote:
> I think it would better to add it, because
> 
> ```
>   message GetFileContents {
> repeated bytes data = 1;
>   }
> ```
> 
> is the response for `Call::READ_FILE`.
> 
> haosdent huang wrote:
> I add the missing `The response for 'Call::GET_X`, do you think we should 
> add them only when necessary?

Yeah, I was thinking of adding these when we _actually_ need them to resolve 
ambiguities e.g., if the response message is different from the call name etc. 

It's fine to add it here for consistency. We can always do a sweep in another 
patch to remove them if we feel the need.


- Anand


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


On June 25, 2016, 6:44 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49157/
> ---
> 
> (Updated June 25, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta, Anand Mazumdar, zhou xing, Jay 
> Guo, Shuai Lin, and Vinod Kone.
> 
> 
> Bugs: MESOS-5695
> https://issues.apache.org/jira/browse/MESOS-5695
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add missing comments for `GET_ROLES`, `GET_WEIGHTS`, `SUBSCRIBE`,
> `CREATE_VOLUMES`, `DESTROY_VOLUMES`, `SET_QUOTA` in operator API
> protos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
> 
> Diff: https://reviews.apache.org/r/49157/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 48320: Update C++ style checker to prevent NULL usage.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48728, 48320]

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

- Mesos ReviewBot


On June 26, 2016, 2:16 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48320/
> ---
> 
> (Updated June 26, 2016, 2:16 p.m.)
> 
> 
> Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3243
> https://issues.apache.org/jira/browse/MESOS-3243
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update C++ style checker to prevent NULL usage.
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py 96e7fbcfe84067becbac89de909a2c6bdf99a60b 
>   support/mesos-style.py 35ef7867c45afa02f0c74b86149f2e0489fe2d15 
> 
> Diff: https://reviews.apache.org/r/48320/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 48320: Update C++ style checker to prevent NULL usage.

[Tomasz Janiszewski]

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

(Updated June 26, 2016, 2:16 p.m.)


Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
Conway.


Changes
---

Rebase on latest changes of 48728. Remove trailing spaces


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


Repository: mesos


Description
---

Update C++ style checker to prevent NULL usage.


Diffs (updated)
-

  support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
  support/cpplint.py 96e7fbcfe84067becbac89de909a2c6bdf99a60b 
  support/mesos-style.py 35ef7867c45afa02f0c74b86149f2e0489fe2d15 

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


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 48728: Updated support/cpplint.patch.

[Tomasz Janiszewski]

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

(Updated June 26, 2016, 2:12 p.m.)


Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
Conway.


Repository: mesos


Description (updated)
---

Generate `support/cpplint.patch` using following command:
```
PATCH="support/cpplint.patch"
FIRST_COMMIT=`git log --pretty=format:"%h"  --diff-filter=A -- $PATCH`
git diff ${FIRST_COMMIT}..master  support/cpplint.py > $PATCH
sed -i 's/[ \t]*$//' "$PATCH"
```


Diffs
-

  support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 

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


Testing
---

```
git co $FIRST_COMMIT
git co HEAD@{1} $PATCH
git apply $PATCH
git diff --exit-code master support/cpplint.py
```


Thanks,

Tomasz Janiszewski

Re: Review Request 48728: Updated support/cpplint.patch.

[Tomasz Janiszewski]

> On June 26, 2016, 8:31 a.m., Michael Park wrote:
> > support/cpplint.patch, line 7
> > 
> >
> > The trailing whitespaces are giving me issues in applying these 
> > patches. Could you rebase and remove the trailing whitespaces, please? Here 
> > and below.

I'm sorry. diff creates this trailing lines and I was not sure if it's safe to 
delete them.


- Tomasz


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


On June 26, 2016, 1:58 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48728/
> ---
> 
> (Updated June 26, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Generate `support/cpplint.patch` using following command:
> ```
> PATCH="support/cpplint.patch"
> FIRST_COMMIT=`git log --pretty=format:"%h"  --diff-filter=A -- $PATCH`
> git diff ${FIRST_COMMIT}..master  support/cpplint.py > $PATCH
> sed -i 's/[ \t]*$//' "$PATCH"
> ```
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
> 
> Diff: https://reviews.apache.org/r/48728/diff/
> 
> 
> Testing
> ---
> 
> ```
> git co $FIRST_COMMIT
> git co HEAD@{1} $PATCH
> git apply $PATCH
> git diff --exit-code master support/cpplint.py
> ```
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 48728: Updated support/cpplint.patch.

[Tomasz Janiszewski]

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

(Updated June 26, 2016, 1:58 p.m.)


Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
Conway.


Changes
---

Remove trailing lines


Repository: mesos


Description (updated)
---

Generate `support/cpplint.patch` using following command:
```
PATCH="support/cpplint.patch"
FIRST_COMMIT=`git log --pretty=format:"%h"  --diff-filter=A -- $PATCH`
git diff ${FIRST_COMMIT}..master  support/cpplint.py > $PATCH
sed -i 's/[ \t]*$//' "$PATCH"
```


Diffs (updated)
-

  support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 

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


Testing (updated)
---

```
git co $FIRST_COMMIT
git co HEAD@{1} $PATCH
git apply $PATCH
git diff --exit-code master support/cpplint.py
```


Thanks,

Tomasz Janiszewski

Re: Review Request 49140: Added startsWith/endsWith to support char.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49140]

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

- Mesos ReviewBot


On June 26, 2016, 9:46 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49140/
> ---
> 
> (Updated June 26, 2016, 9:46 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-5692
> https://issues.apache.org/jira/browse/MESOS-5692
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added startsWith/endsWith to support char.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 
> 
> Diff: https://reviews.apache.org/r/49140/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 48320: Update C++ style checker to prevent NULL usage.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [48320, 48728]

Failed command: ./support/apply-review.sh -n -r 48728

Error:
2016-06-26 10:07:51 URL:https://reviews.apache.org/r/48728/diff/raw/ 
[6144/6144] -> "48728.patch" [1]
48728.patch:22: trailing whitespace.
 
48728.patch:55: trailing whitespace.
 
48728.patch:56: trailing whitespace.
 
warning: 3 lines add whitespace errors.
support/cpplint.patch:33: trailing whitespace.
+ 
support/cpplint.patch:34: trailing whitespace.
+ 

Full log: https://builds.apache.org/job/mesos-reviewbot/13996/console

- Mesos ReviewBot


On June 15, 2016, 12:55 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48320/
> ---
> 
> (Updated June 15, 2016, 12:55 p.m.)
> 
> 
> Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3243
> https://issues.apache.org/jira/browse/MESOS-3243
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update C++ style checker to prevent NULL usage.
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py 96e7fbcfe84067becbac89de909a2c6bdf99a60b 
>   support/mesos-style.py 35ef7867c45afa02f0c74b86149f2e0489fe2d15 
> 
> Diff: https://reviews.apache.org/r/48320/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 49140: Added startsWith/endsWith to support char.

[Klaus Ma]

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

(Updated June 26, 2016, 5:46 p.m.)


Review request for mesos and Michael Park.


Changes
---

rebase


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


Repository: mesos


Description
---

Added startsWith/endsWith to support char.


Diffs (updated)
-

  3rdparty/stout/include/stout/strings.hpp 
162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 

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


Testing
---

make


Thanks,

Klaus Ma

Re: Review Request 49140: Added startsWith/endsWith to support char.

[Klaus Ma]

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

(Updated June 26, 2016, 5:40 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address mycpark's comments.


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


Repository: mesos


Description
---

Added startsWith/endsWith to support char.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 
  3rdparty/stout/include/stout/strings.hpp 
162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 
  docs/contributors.yaml 083715f23bb2d0610d94dd8995a026254125 
  docs/home.md 2d728333a3c1421b310d861b71c92691fd41a482 
  docs/networking.md f6652f58b02edee08e0b2410c23b2beb4d25e83b 
  docs/port-mapping-isolator.md 6a6bab9df3f6f3960ceeab38b6302823dceae9c2 
  docs/submitting-a-patch.md ffc6e561b8721b8849ef6025c15936ea712d3bfa 
  src/Makefile.am 61789a7603dac08a5f8ac4fe3d63b43e123ed98a 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 5cc9e86bc7966051507ce6620651d1f5d0914994 
  src/files/files.cpp a5a1b86e14f63e5e3834a2900270252fbe16f586 
  src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
  src/master/master.cpp d89c049326358bad509bb1455c81eb11610eeeb8 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
769dfa242ed5322a72b65fbb422894e70af2ad0c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
92b33111799cb4e1c8bc2051381e1254d701d95c 
  src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 
7678a7c81c3cdb27410c1f066021eb34bd02a83f 
  src/slave/containerizer/mesos/isolators/network/cni/paths.cpp 
f9056c90f1075cb19c4f554e7d5b629561d06035 
  src/slave/http.cpp c038bf0c9680ec86f77f1a27efeb7354a9e67627 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 
  src/tests/containerizer/cni_isolator_tests.cpp 
991823b96f8eac7539625ef0f1e045e6a10464ac 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
28ec3f9954576d78153e9d0f57e22a240e950639 
  src/tests/files_tests.cpp 31337e280c6224a8c949c8868a53e5a785b4573f 
  src/tests/mesos.hpp bcd060ec07da27db8738950bd81ead37da11ee70 
  src/tests/mesos.cpp 6918343b13a735aec243b54a9fcbced056894f53 
  src/tests/persistent_volume_endpoints_tests.cpp 
6c85e19eeaa69bf3a4e3077261331191db6eec06 
  src/tests/reservation_endpoints_tests.cpp 
3ee59d5db0089dd59acfe48a77910d069ffc377b 
  src/tests/slave_authorization_tests.cpp 
18bcb0e499a9d2d84113b5b9e609e5e40913ebcc 
  support/docker_build.sh 8ae1aadbc12b12e44984d34ccfbcb8a97bf05bcf 

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


Testing
---

make


Thanks,

Klaus Ma

Re: Review Request 46687: Updated legacy `dispatch` calls to use lambda.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46686, 46687]

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

- Mesos ReviewBot


On June 24, 2016, 8:57 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46687/
> ---
> 
> (Updated June 24, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Michael Park.
> 
> 
> Bugs: MESOS-4611
> https://issues.apache.org/jira/browse/MESOS-4611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated legacy `dispatch` calls to use lambda.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 4337e6028a3a6e5279793c7c6f73bb9a4f60cb0a 
> 
> Diff: https://reviews.apache.org/r/46687/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49140: Added startsWith/endsWith to support char.

[Michael Park]

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




3rdparty/stout/include/stout/strings.hpp (line 376)


Let's use `s.front()` rather than `*s.begin()`.



3rdparty/stout/include/stout/strings.hpp (line 388)


Let's use `s.back()` rather than `*s.rbegin()`.


- Michael Park


On June 23, 2016, 2:51 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49140/
> ---
> 
> (Updated June 23, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-5692
> https://issues.apache.org/jira/browse/MESOS-5692
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added startsWith/endsWith to support char.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 
> 
> Diff: https://reviews.apache.org/r/49140/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 48320: Update C++ style checker to prevent NULL usage.

[Michael Park]

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


Fix it, then Ship it!





support/cpplint.patch (line 102)


The trailing whitespaces are giving me issues in applying these patches. 
Could you rebase and remove the trailing whitespaces, please?


- Michael Park


On June 15, 2016, 12:55 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48320/
> ---
> 
> (Updated June 15, 2016, 12:55 p.m.)
> 
> 
> Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3243
> https://issues.apache.org/jira/browse/MESOS-3243
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update C++ style checker to prevent NULL usage.
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py 96e7fbcfe84067becbac89de909a2c6bdf99a60b 
>   support/mesos-style.py 35ef7867c45afa02f0c74b86149f2e0489fe2d15 
> 
> Diff: https://reviews.apache.org/r/48320/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 48728: Updated support/cpplint.patch.

[Michael Park]

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


Fix it, then Ship it!





support/cpplint.patch (line 7)


The trailing whitespaces are giving me issues in applying these patches. 
Could you rebase and remove the trailing whitespaces, please? Here and below.


- Michael Park


On June 15, 2016, 12:49 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48728/
> ---
> 
> (Updated June 15, 2016, 12:49 p.m.)
> 
> 
> Review request for mesos, Adam Berry, Benjamin Mahler, Michael Park, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Generate `support/cpplint.patch` using following command:
> ```
> PATCH="support/cpplint.patch"
> FIRST_COMMIT=`git log --pretty=format:"%h"  --diff-filter=A -- $PATCH`
> git diff ${FIRST_COMMIT}..master  support/cpplint.py > $PATCH
> ```
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
> 
> Diff: https://reviews.apache.org/r/48728/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 49088: Replaced default capture by value by explicit capture by value.

[Michael Park]

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


Fix it, then Ship it!





src/master/http.cpp (lines 1922 - 1924)


Let's maintain the formatting here.


- Michael Park


On June 25, 2016, 12:33 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49088/
> ---
> 
> (Updated June 25, 2016, 12:33 p.m.)
> 
> 
> Review request for mesos, Jay Guo, Joris Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When default capture by value is used in a lambda, it's hard
> to reason within what context the lambda has to be executed
> without auditing the code.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
> 
> Diff: https://reviews.apache.org/r/49088/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49062: Fixed lambda capture list for consistency.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 25, 2016, 12:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49062/
> ---
> 
> (Updated June 25, 2016, 12:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Our jsonify library consumes the lambdas immediately, without saving
> them internally or running asynchronously. Hence it is OK to capture
> by reference in this case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
>   src/slave/http.cpp c038bf0c9680ec86f77f1a27efeb7354a9e67627 
> 
> Diff: https://reviews.apache.org/r/49062/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49061: Removed the unused copy of an `ObjectApprover` in a lambda.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 25, 2016, 12:31 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49061/
> ---
> 
> (Updated June 25, 2016, 12:31 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
> 
> Diff: https://reviews.apache.org/r/49061/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49060: Removed unused approver from tasks authorization continuation.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 25, 2016, 12:35 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49060/
> ---
> 
> (Updated June 25, 2016, 12:35 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ad3f723ec21dd0e9f3a3f015d4de0702625cd603 
> 
> Diff: https://reviews.apache.org/r/49060/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 32058: Added protobuf-JSON validation

[Akanksha Agrawal]

> On April 16, 2015, 2:46 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 573
> > 
> >
> > Just as Vinod commented on the JIRA, we might want  to do that 
> > differently - e.g. by adding a factory.

I did not understand how this should be done. Can you elaborate it a bit?


- Akanksha


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


On March 16, 2015, 8:02 p.m., Akanksha Agrawal wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32058/
> ---
> 
> (Updated March 16, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-1194
> https://issues.apache.org/jira/browse/MESOS-1194
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf-JSON validation
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 2c020d98b94995bce862ac3b9c173dd64ed1198d 
> 
> Diff: https://reviews.apache.org/r/32058/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akanksha Agrawal
> 
>

Re: Review Request 46687: Updated legacy `dispatch` calls to use lambda.

[Michael Park]

> On June 26, 2016, 7:37 a.m., Michael Park wrote:
> > Could you specify that this only updates `http_tests.cpp`? There are other 
> > places in the codebase where we explicitly construct a `std::function` 
> > first, but we don't update all of those in this patch.

Just in the summary and/or description, that is.


- Michael


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


On June 24, 2016, 8:57 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46687/
> ---
> 
> (Updated June 24, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Michael Park.
> 
> 
> Bugs: MESOS-4611
> https://issues.apache.org/jira/browse/MESOS-4611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated legacy `dispatch` calls to use lambda.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 4337e6028a3a6e5279793c7c6f73bb9a4f60cb0a 
> 
> Diff: https://reviews.apache.org/r/46687/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46687: Updated legacy `dispatch` calls to use lambda.

[Michael Park]

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


Fix it, then Ship it!




Could you specify that this only updates `http_tests.cpp`? There are other 
places in the codebase where we explicitly construct a `std::function` first, 
but we don't update all of those in this patch.


3rdparty/libprocess/src/tests/http_tests.cpp (lines 192 - 194)


This fits in one line.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 264 - 266)


Also fits in one line.


- Michael Park


On June 24, 2016, 8:57 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46687/
> ---
> 
> (Updated June 24, 2016, 8:57 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Michael Park.
> 
> 
> Bugs: MESOS-4611
> https://issues.apache.org/jira/browse/MESOS-4611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated legacy `dispatch` calls to use lambda.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 4337e6028a3a6e5279793c7c6f73bb9a4f60cb0a 
> 
> Diff: https://reviews.apache.org/r/46687/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46686: Allowed to pass lambda in `dispatch`.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/dispatch.hpp (line 72)


```
template 
struct Dispatch;



3rdparty/libprocess/include/process/dispatch.hpp (lines 75 - 76)


```
// Partial specialization for callable objects returning `void` to be 
dispatched
// on a process.
```



3rdparty/libprocess/include/process/dispatch.hpp (line 82)


Let's not force a construction of `std::function` here, since the `F&&` 
being forwarded could be a lambda or other callable objects.

```
- void operator()(const UPID& pid, const std::function& f)
+ template 
+ void operator()(const UPID& pid, F&& f)
```



3rdparty/libprocess/include/process/dispatch.hpp (lines 95 - 96)


```
// Partial specialization for callable objects returning `Future` to be
// dispatched on a process.
```



3rdparty/libprocess/include/process/dispatch.hpp (line 102)


Same comment as the `void` case.



3rdparty/libprocess/include/process/dispatch.hpp (line 119)


`s/other types/R/`



3rdparty/libprocess/include/process/dispatch.hpp (lines 119 - 120)


```
// Dispatches a callable object returning `R` on a process.
```



3rdparty/libprocess/include/process/dispatch.hpp (line 126)


Same comment as the `void` case.


- Michael Park


On June 24, 2016, 8:56 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46686/
> ---
> 
> (Updated June 24, 2016, 8:56 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Michael Park.
> 
> 
> Bugs: MESOS-4611
> https://issues.apache.org/jira/browse/MESOS-4611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed to pass lambda in `dispatch`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 9e36047bfde39ccfdd5151639931a46d24e419c8 
> 
> Diff: https://reviews.apache.org/r/46686/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>