You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/09/17 02:27:23 UTC
Review Request 38443: Added layerid information to ManifestResponse
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
Review request for mesos and Timothy Chen.
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99346
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38443]
All tests passed.
- Mesos ReviewBot
On Sept. 17, 2015, 12:27 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 12:27 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99926
-----------------------------------------------------------
src/slave/containerizer/provisioner/docker/registry_client.cpp (line 291)
<https://reviews.apache.org/r/38443/#comment156940>
Failed to parse error message: Error expected to be JSON object.
I think next iteration we should convert all these JSON parsing into using a protobuf object.
- Timothy Chen
On Sept. 18, 2015, 1:17 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 1:17 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99472
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38443]
All tests passed.
- Mesos ReviewBot
On Sept. 18, 2015, 1:17 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2015, 1:17 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review100856
-----------------------------------------------------------
src/slave/containerizer/provisioner/docker/registry_client.cpp (line 79)
<https://reviews.apache.org/r/38443/#comment158103>
tokenMgr -> tokenManager, I can fix myself
src/slave/containerizer/provisioner/docker/registry_client.cpp (line 505)
<https://reviews.apache.org/r/38443/#comment158106>
I think we need to say we failed because it's not the expected Object type. Same here and other placess you have the check
Also you use index: in other places, and just index here. Let's be consistent and use index:
- Timothy Chen
On Sept. 25, 2015, 6:33 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2015, 6:33 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 13, 2015, 12:27 a.m., Timothy Chen wrote:
> > Ship It!
Tim if this looks good to you can you commit? I'd like to work through the cleanup chain that follows this :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review102363
-----------------------------------------------------------
On Oct. 9, 2015, 6:34 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 6:34 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567
> src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285
> src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review102363
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Oct. 9, 2015, 6:34 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 6:34 p.m.)
>
>
> Review request for mesos, Ben Mahler and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567
> src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285
> src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Oct. 9, 2015, 6:34 p.m.)
Review request for mesos, Ben Mahler and Timothy Chen.
Changes
-------
Split change into small patches
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp fdb68b675582f603ffb3e96f31c1c9405e238567
src/slave/containerizer/provisioner/docker/registry_client.cpp 4931ae8869a697b1e9d8d4cbc0a871e7cd506285
src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Oct. 6, 2015, 2:31 a.m.)
Review request for mesos and Timothy Chen.
Changes
-------
reordered patches
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd
src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Oct. 2, 2015, 3:38 a.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Rebased with master
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp c2040b48ea43fdb29766994c244273d3fa9ee3cd
src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Sept. 28, 2015, 10:50 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
review comments addressed.
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Sept. 25, 2015, 6:33 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
rebased with master
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp d895eb9d0723e52cff8b21ef2deeaef1911d019c
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Sept. 22, 2015, 11:37 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
addressed review comments.
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Sept. 18, 2015, 1:17 a.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Added guard for as<JSON::Object>. Also moved arguments around.
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99411
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38443]
All tests passed.
- Mesos ReviewBot
On Sept. 17, 2015, 5:06 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 5:06 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/
-----------------------------------------------------------
(Updated Sept. 17, 2015, 5:06 p.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Added guard for array sizes.
Repository: mesos
Description
-------
Added layerid information to ManifestResponse
Diffs (updated)
-----
src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
Diff: https://reviews.apache.org/r/38443/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Sept. 17, 2015, 2:54 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 499
> > <https://reviews.apache.org/r/38443/diff/1/?file=1075821#file1075821line499>
> >
> > If history array contains non JSON Object from the response I think it will just crash the slave.
> >
> > I realize in other places too, this becomes a bit tricky when docker registry changes their impl, or when user is targeting some other version of the registry I tihnk we shouldn't crash the whole slave.
if you mean the array sizes differences between history and fslayers then i have fixed the patch for that.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99351
-----------------------------------------------------------
On Sept. 17, 2015, 5:06 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 5:06 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38443: Added layerid information to
ManifestResponse
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38443/#review99351
-----------------------------------------------------------
src/slave/containerizer/provisioner/docker/registry_client.cpp (line 499)
<https://reviews.apache.org/r/38443/#comment156242>
If history array contains non JSON Object from the response I think it will just crash the slave.
I realize in other places too, this becomes a bit tricky when docker registry changes their impl, or when user is targeting some other version of the registry I tihnk we shouldn't crash the whole slave.
- Timothy Chen
On Sept. 17, 2015, 12:27 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38443/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 12:27 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added layerid information to ManifestResponse
>
>
> Diffs
> -----
>
> src/slave/containerizer/provisioner/docker/registry_client.hpp 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185
> src/slave/containerizer/provisioner/docker/registry_client.cpp 0a9663139b8a6d9e4fa458f7c893d33ca3a30e90
> src/tests/containerizer/provisioner_docker_tests.cpp 1b0c30450a07d01d1a38d460bc7d921c80be7e3b
>
> Diff: https://reviews.apache.org/r/38443/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>