You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Aditi Dixit <ad...@gmail.com> on 2015/07/09 18:04:29 UTC
Re: Review Request 35687: Added capabilities to state.json
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/
-----------------------------------------------------------
(Updated July 9, 2015, 4:04 p.m.)
Review request for mesos, Marco Massenzio and Vinod Kone.
Bugs: MESOS-2900
https://issues.apache.org/jira/browse/MESOS-2900
Repository: mesos
Description (updated)
-------
Added capabilities to state.json and test for the same
Diffs (updated)
-----
src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/35687/diff/
Testing
-------
make check
Thanks,
Aditi Dixit
Re: Review Request 35687: Added capabilities to state.json
Posted by Aditi Dixit <ad...@gmail.com>.
> On July 9, 2015, 4:26 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 124-125
> > <https://reviews.apache.org/r/35687/diff/2/?file=1003411#file1003411line124>
> >
> > were you not able to use 'foreach' here?
> >
> > foreach(FrameworkInfo::Capability& capability, framework.info.capabilities()) {
> >
> > }
No. I think the problem is occurring because capabilities is a repeated field.
- Aditi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91113
-----------------------------------------------------------
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91113
-----------------------------------------------------------
src/master/http.cpp (lines 124 - 125)
<https://reviews.apache.org/r/35687/#comment144379>
were you not able to use 'foreach' here?
foreach(FrameworkInfo::Capability& capability, framework.info.capabilities()) {
}
src/tests/master_tests.cpp (lines 2994 - 2996)
<https://reviews.apache.org/r/35687/#comment144380>
pull this below #2998 so that webui_url testing is all in one block.
also, you captured 'Capability' into an JSON::array but doing nothing with it? did you forget to add an EXPECT_EQ to make sure this value is the same as the one you added in #2958?
- Vinod Kone
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On July 12, 2015, 10:48 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 124-125
> > <https://reviews.apache.org/r/35687/diff/2/?file=1003411#file1003411line124>
> >
> > Hm, I wonder why didn't it work, did you get a compilation error? For example, we have the following code in the master, which compiles fine:
> > ```
> > case Offer::Operation::LAUNCH: {
> > foreach (const TaskInfo& task, operation.launch().task_infos()) {
> > ...
> > ```
To @Vinod's comment above.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91408
-----------------------------------------------------------
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91408
-----------------------------------------------------------
src/master/http.cpp (lines 124 - 125)
<https://reviews.apache.org/r/35687/#comment144779>
Hm, I wonder why didn't it work, did you get a compilation error? For example, we have the following code in the master, which compiles fine:
```
case Offer::Operation::LAUNCH: {
foreach (const TaskInfo& task, operation.launch().task_infos()) {
...
```
- Alexander Rukletsov
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91128
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35687]
All tests passed.
- Mesos ReviewBot
On July 9, 2015, 4:04 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 9, 2015, 4:04 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91436
-----------------------------------------------------------
src/master/http.cpp (line 124)
<https://reviews.apache.org/r/35687/#comment144809>
Please add a space between `foreach` and a bracket for consistency: `foreach (...`
src/tests/master_tests.cpp (line 2949)
<https://reviews.apache.org/r/35687/#comment144811>
We tend to limit comments to 70 chars, this line looks longer. Mind wrapping it?
src/tests/master_tests.cpp (line 2997)
<https://reviews.apache.org/r/35687/#comment144817>
We use `lowerCamelCase` for variable names, please rename.
src/tests/master_tests.cpp (line 3001)
<https://reviews.apache.org/r/35687/#comment144814>
Instead of the hardcoded constant, it makes sense to use `FrameworkInfo::Capability::Type_Name()` IMO.
- Alexander Rukletsov
On July 12, 2015, 6:25 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 6:25 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91414
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35687]
All tests passed.
- Mesos ReviewBot
On July 12, 2015, 6:25 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 6:25 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91508
-----------------------------------------------------------
LGTM modulo Alex's reviews. Fix them and I'll commit this for you.
- Vinod Kone
On July 12, 2015, 6:25 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 6:25 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91789
-----------------------------------------------------------
Ship it!
minor nits. i'll fix them and commit it for you. thanks.
src/master/http.cpp (line 125)
<https://reviews.apache.org/r/35687/#comment145437>
'framework' should be aligned with 'const'.
src/tests/master_tests.cpp (line 2957)
<https://reviews.apache.org/r/35687/#comment145439>
s/capabilityName/capabilityType/
- Vinod Kone
On July 15, 2015, 5:20 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 5:20 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/#review91774
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35687]
All tests passed.
- Mesos ReviewBot
On July 15, 2015, 5:20 p.m., Aditi Dixit wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35687/
> -----------------------------------------------------------
>
> (Updated July 15, 2015, 5:20 p.m.)
>
>
> Review request for mesos, Marco Massenzio and Vinod Kone.
>
>
> Bugs: MESOS-2900
> https://issues.apache.org/jira/browse/MESOS-2900
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added capabilities to state.json and test for the same
>
>
> Diffs
> -----
>
> src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
> src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
>
> Diff: https://reviews.apache.org/r/35687/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Aditi Dixit
>
>
Re: Review Request 35687: Added capabilities to state.json
Posted by Aditi Dixit <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/
-----------------------------------------------------------
(Updated July 15, 2015, 5:20 p.m.)
Review request for mesos, Marco Massenzio and Vinod Kone.
Bugs: MESOS-2900
https://issues.apache.org/jira/browse/MESOS-2900
Repository: mesos
Description
-------
Added capabilities to state.json and test for the same
Diffs (updated)
-----
src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/35687/diff/
Testing
-------
make check
Thanks,
Aditi Dixit
Re: Review Request 35687: Added capabilities to state.json
Posted by Aditi Dixit <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35687/
-----------------------------------------------------------
(Updated July 12, 2015, 6:25 p.m.)
Review request for mesos, Marco Massenzio and Vinod Kone.
Bugs: MESOS-2900
https://issues.apache.org/jira/browse/MESOS-2900
Repository: mesos
Description
-------
Added capabilities to state.json and test for the same
Diffs (updated)
-----
src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332
src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22
Diff: https://reviews.apache.org/r/35687/diff/
Testing
-------
make check
Thanks,
Aditi Dixit