You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2016/11/28 22:50:14 UTC
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 10:50 p.m.)
Review request for mesos, Anand Mazumdar and Benjamin Mahler.
Changes
-------
updated the diff per changs in the dependent reviews.
Bugs: MESOS-6473
https://issues.apache.org/jira/browse/MESOS-6473
Repository: mesos
Description
-------
Added API handler for ATTACH_CONTAINER_OUTPUT.
Diffs (updated)
-----
src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
src/tests/containerizer/mock_containerizer.hpp 7a30b8307b93b7bf549efb52d72367f652d0d95a
Diff: https://reviews.apache.org/r/53995/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/#review157190
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54048, 54093, 54039, 53994, 54049, 53995]
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 Nov. 28, 2016, 10:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
>
>
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added API handler for ATTACH_CONTAINER_OUTPUT.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
> src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
> src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
> src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
> src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
> src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
> src/tests/containerizer/mock_containerizer.hpp 7a30b8307b93b7bf549efb52d72367f652d0d95a
>
> Diff: https://reviews.apache.org/r/53995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/
-----------------------------------------------------------
(Updated Nov. 29, 2016, 6:11 a.m.)
Review request for mesos, Anand Mazumdar and Benjamin Mahler.
Changes
-------
anand's. NNFR.
Bugs: MESOS-6473
https://issues.apache.org/jira/browse/MESOS-6473
Repository: mesos
Description
-------
Added API handler for ATTACH_CONTAINER_OUTPUT.
Diffs (updated)
-----
src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
Diff: https://reviews.apache.org/r/53995/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/#review157204
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/http.cpp (line 2175)
<https://reviews.apache.org/r/53995/#comment227661>
argument by reference: `Future<Nothing>&`
src/tests/api_tests.cpp (line 31)
<https://reviews.apache.org/r/53995/#comment227660>
Remove this include?
- Anand Mazumdar
On Nov. 29, 2016, 5:38 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 5:38 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
>
>
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added API handler for ATTACH_CONTAINER_OUTPUT.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
> src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
> src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
> src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
> src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
> src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
>
> Diff: https://reviews.apache.org/r/53995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/
-----------------------------------------------------------
(Updated Nov. 29, 2016, 5:38 a.m.)
Review request for mesos, Anand Mazumdar and Benjamin Mahler.
Changes
-------
anand's comments. thanks for the thorough review!
also, fixed the content type of the response.
Bugs: MESOS-6473
https://issues.apache.org/jira/browse/MESOS-6473
Repository: mesos
Description
-------
Added API handler for ATTACH_CONTAINER_OUTPUT.
Diffs (updated)
-----
src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
Diff: https://reviews.apache.org/r/53995/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 53995: Added API handler for
ATTACH_CONTAINER_OUTPUT.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53995/#review157165
-----------------------------------------------------------
Looks good! Just some minor comments around style/questions around implementation.
src/internal/evolve.hpp (line 80)
<https://reviews.apache.org/r/53995/#comment227596>
Can you move this method after L78 to sort them by return type as we did for the other overloads in this file?
You would also need to move the method in the .cpp file
src/slave/http.cpp (line 89)
<https://reviews.apache.org/r/53995/#comment227597>
Move this above L88?
src/slave/http.cpp (line 2119)
<https://reviews.apache.org/r/53995/#comment227599>
4 space indent here
src/slave/http.cpp (line 2125)
<https://reviews.apache.org/r/53995/#comment227600>
Nit: s/'HOST'/The 'HOST'
src/slave/http.cpp (lines 2131 - 2132)
<https://reviews.apache.org/r/53995/#comment227598>
hmm, it's somewhat weird that we set it to `/api`. Why not `/switchboard` or anything else.
I would just set it to `/` i.e., the server root since currently i.e. the only path we support on the server. We can then make the switchboard implementation validate it. Is there an issue if we set it to `/`?
src/slave/http.cpp (line 2138)
<https://reviews.apache.org/r/53995/#comment227601>
Can you be explicit here? (You don't need access to `this` here and it can lead to subtle bugs in the future)
src/slave/http.cpp (line 2143)
<https://reviews.apache.org/r/53995/#comment227602>
Quotes before `ProcessIO`
src/slave/http.cpp (lines 2145 - 2148)
<https://reviews.apache.org/r/53995/#comment227605>
You might want to reorder these statements as:
```cpp
Pipe pipe;
Pipe::Writer writer = pipe.writer();
OK ok;
ok.reader = pipe.reader();
```
src/slave/http.cpp (line 2160)
<https://reviews.apache.org/r/53995/#comment227607>
Do you want to capture everything here?
src/slave/http.cpp (line 2168)
<https://reviews.apache.org/r/53995/#comment227608>
hmm, you would need to capture `Connection` here in this lambda. I don't quite understand how it works if you don't do that i.e., the destructor of `Connection` would be called since there is no reference to it remaining that would in turn result in the receiving side of the socket closing i.e., it won't accept any more streaming responses!
src/tests/api_tests.cpp (line 80)
<https://reviews.apache.org/r/53995/#comment227609>
Why do you need this using declaration and the corresponding header?
src/tests/api_tests.cpp
<https://reviews.apache.org/r/53995/#comment227611>
hmm, why did you remove this using declaration?
src/tests/containerizer/mock_containerizer.hpp (lines 71 - 73)
<https://reviews.apache.org/r/53995/#comment227610>
hmm, not sure why did you need the change in this review? You don't seem to be using the `MockContainerizer` in your test?
- Anand Mazumdar
On Nov. 28, 2016, 10:50 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53995/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Benjamin Mahler.
>
>
> Bugs: MESOS-6473
> https://issues.apache.org/jira/browse/MESOS-6473
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added API handler for ATTACH_CONTAINER_OUTPUT.
>
>
> Diffs
> -----
>
> src/internal/evolve.hpp e8b03c9f04b8a835f35ce7cb6cc44e3ecd799422
> src/internal/evolve.cpp 23a515c0aa8c31e0789fab84830dae32574604bc
> src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f
> src/slave/slave.hpp 0cc105418bf4e35b604ebe412048237814e495fb
> src/slave/validation.cpp b3d8bcd0f0855d2a0a96821900d996ba86e11578
> src/tests/api_tests.cpp 8889e7807ecead736eaac3910332e86d594e8cec
> src/tests/containerizer/mock_containerizer.hpp 7a30b8307b93b7bf549efb52d72367f652d0d95a
>
> Diff: https://reviews.apache.org/r/53995/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>