Re: Review Request 38579: Refactored registry client: split large methods.

[Jojy Varghese]

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

(Updated Nov. 5, 2015, 5:57 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Summary (updated)
-

Refactored registry client: split large methods.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client: split large methods.

[Jojy Varghese]

> On Nov. 5, 2015, 5:54 p.m., Timothy Chen wrote:
> > Can you update the title to something more specific of what this patch does?

updated.


- Jojy


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


On Nov. 5, 2015, 5:57 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Nov. 5, 2015, 5:57 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Cleanup and broke big methods into smaller chunks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 5, 2015, 4:03 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Timothy Chen]

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


Can you update the title to something more specific of what this patch does?

- Timothy Chen


On Nov. 5, 2015, 4:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Nov. 5, 2015, 4:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Cleanup and broke big methods into smaller chunks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client: split large methods.

[Jojy Varghese]

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

(Updated Nov. 5, 2015, 7:17 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client: split large methods.

[Timothy Chen]

> On Nov. 6, 2015, 2:54 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line 
> > 529
> > 
> >
> > Can you use the new .status == http::Status::OK and simliar checks here?

Never mind let's worry about this later.


- Timothy


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


On Nov. 5, 2015, 7:17 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Nov. 5, 2015, 7:17 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Cleanup and broke big methods into smaller chunks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client: split large methods.

[Jojy Varghese]

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

(Updated Nov. 6, 2015, 6:57 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

review comments


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client: split large methods.

[Timothy Chen]

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



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (line 276)


Remove redundant check from above.



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (line 291)


None of our onX callbacks with futures should return anything, since it 
has no effect!

Just remove this onDiscard if you're not really doing anything here.



src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp (line 445)


Can you use the new .status == http::Status::OK and simliar checks here?


- Timothy Chen


On Nov. 5, 2015, 7:17 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Nov. 5, 2015, 7:17 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Cleanup and broke big methods into smaller chunks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> ad57d8592c5f9df5115b575855ed2e99a0597359 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 8d90894410cd834edf49a2814d1b616718798fe8 
> 
> Diff: https://reviews.apache.org/r/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 6:07 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

moved timeout semantics to the caller.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
8d90894410cd834edf49a2814d1b616718798fe8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 2, 2015, 10:45 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Nov. 1, 2015, 3:28 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
ad57d8592c5f9df5115b575855ed2e99a0597359 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 14, 2015, 4:06 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 
  src/tests/containerizer/provisioner_docker_tests.cpp 
9c3c45a81be6398722a37911788e347a4e91cce8 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 13, 2015, 7:18 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 9, 2015, 6:36 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 9, 2015, 12:58 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

created more sub-patches.


Repository: mesos


Description
---

- Cleanup and broke big methods into smaller chunks.


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
4931ae8869a697b1e9d8d4cbc0a871e7cd506285 
  src/tests/containerizer/provisioner_docker_tests.cpp 
d895eb9d0723e52cff8b21ef2deeaef1911d019c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39013, 38443, 38579]

All tests passed.

- Mesos ReviewBot


On Oct. 6, 2015, 2:33 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 6, 2015, 2:33 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Cleanup and broke big methods into smaller chunks.
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 5, 2015, 9:23 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

Reordered patches.


Repository: mesos


Description
---

- Moved ManifestResponse struct from RegistryClient to namespace.
- Cleanup


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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 6, 2015, 2:32 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

reordered patches.


Repository: mesos


Description
---

- Moved ManifestResponse struct from RegistryClient to namespace.
- Cleanup


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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 6, 2015, 2:33 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

updated description.


Repository: mesos


Description (updated)
---

- Cleanup and broke big methods into smaller chunks.


Diffs
-

  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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.hpp, lines 36-40
> > 
> >
> > Hm.. any reason the registry client is nested within the containerizer?
> > 
> > A registry client seems like a general abstraction, should it live up 
> > alongside the `Docker` abstraction within src/docker?

There is more to this decision than what meets the eye :). I will give it a 
shot to explain. registry client and tokenmanager does "registry" specific 
things. They serve the privisioner right now. Registry namespace was created to 
highlight the regitry domain for provisioners. There was also a thought to keep 
the registry classes in its own directory. But that was later undone. Since the 
classes RegistryClient and TokenManager now lie in the provisioner dierctory, 
the file names are fully qualified. 

That is the "descriptive" explanation but is not normative.


- Jojy


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


On Oct. 2, 2015, 12:24 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 2, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Timothy Chen]

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



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 654 - 671)


I believe I left a comment about this before and talked to Joris, we're 
missing the file option that we can stream/splice to disk from libprocess.


- Timothy Chen


On Oct. 2, 2015, 12:24 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 2, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Ben Mahler]

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


Thanks for taking this on Jojy! This is going to be a great exercise in sending 
out small patches so we land things quickly :)

First thing is that this review depends on the addition of new functionality in 
https://reviews.apache.org/r/38443/ . It's easier for the reviewers to help you 
land your cleanups without it being blocked on the new functionality, can you 
remove the dependency?

I wasn't able to do a complete pass on everything since there is a lot of code 
here. But to get things started, I've done an initial pass and made a number of 
comments below. Let's start by addressing them and using small independent 
patches for each cleanup, that way we can make progress without having to clean 
everything up all in one patch.

Much appreciated!!


src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 36 - 40)


Hm.. any reason the registry client is nested within the containerizer?

A registry client seems like a general abstraction, should it live up 
alongside the `Docker` abstraction within src/docker?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 46)


Can we avoid the redundant naming here? We currently have 
`docker::registry::RegistryClient`, how about `docker::registry::Client` or 
`docker::RegistryClient`?



src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 53 - 54)


Hm.. can you fix the style on your comments? There should be a space after 
the `//`.



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 64)


Why ManifestResponse instead of just Manifest..?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 67)


How about: s/fsLayerInfoList/layers/



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 71)


Authentication or authorization? Can you avoid the abbreviation? This is 
what we've done for the rest of our authentication/authorization code.



src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 75 - 86)


As it stands these comments don't seem to be adding any value over what the 
code expresses, should we remove them? Or is there anything that you think they 
should call out that the reader can't see from the variable names?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 100)


authentication or authorization, or both? Can you do a pass for this 
abbreviation?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 147)


We've been trying to eliminate static non-PODs, could you replace this with 
a static function?



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 62 - 65)


Hm.. why did you need this additional static create? Looking at the 
implementation it seems as though you only need `RegistryClient::create`.



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 106)


How about s/getManifestResponse/getManifest/



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 122 - 130)


Generally our pattern for process wrappers is to have the wrapper 
constructor allocate the process. Here it is getting injected. Can you refactor 
this to make it consistent? Ditto for TokenManager.

This means avoiding RegistryClientProcess::create and creating the token 
manager in here, then passing everything we need into the RegistryClient 
constructor, which is a simple pass through to the RegistryClientProcess 
constructor.

Looking at the TokenManager code, I'm confused why TokenManager::create 
exists, and why there is a Try. It looks like token manager construction never 
returns an error?

If we clean up the token manager creation code in the same way as I'm 
suggesting here, we can make this a lot simpler as well. :)



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 193)


We try to avoid abbreviations for names like this, can you do a pass to 
clean them up?



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 231)

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.hpp, lines 75-86
> > 
> >
> > As it stands these comments don't seem to be adding any value over what 
> > the code expresses, should we remove them? Or is there anything that you 
> > think they should call out that the reader can't see from the variable 
> > names?

They only serve doxygen. I thought all new code(header files) has to be 
commented for generating doxygen based docs.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.hpp, line 148
> > 
> >
> > We've been trying to eliminate static non-PODs, could you replace this 
> > with a static function?

I have got rid of this constant in the subsequent patch 
(https://reviews.apache.org/r/38580)


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 63-66
> > 
> >
> > Hm.. why did you need this additional static create? Looking at the 
> > implementation it seems as though you only need `RegistryClient::create`.

Not sure I follow. This create is the factory for the process class. If there 
is an error creating the Process class, this should help in catching that. Isnt 
it?


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 197
> > 
> >
> > We try to avoid abbreviations for names like this, can you do a pass to 
> > clean them up?

I have taken liberty when using commonly used variable names. creds, params etc 
are example of that 
(http://lxr.free-electrons.com/source/include/linux/sched.h). I can change it 
if you feel strongly about it. Also, in the context of Docker its always 
authentication.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 594
> > 
> >
> > Strange that size_t is used here which I'm guessing is why you stuck 
> > the post-decrement in the loop condition. That's a bit tricky, can you just 
> > use an rbegin iterator here?

i would have but i need the index number.


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 548-549
> > 
> >
> > Why is this a member function? Can it be a standalone function for 
> > converting a response into a manifest?

should i leave it as a lambda then?


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 716-718
> > 
> >
> > Why only this validation? Should 'path' be a 'Path'?

No since this is not a filepath but a url path


> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote:
> > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 
> > 724-741
> > 
> >
> > Is it acceptable to hold the whole blob in memory like this?? How big 
> > can these blobs be?

Thats a good point. Ideally we should have a buffered socker reader. Would 
appreciate if you could point me to an example of buffered reader from http.


- Jojy


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


On Oct. 2, 2015, 12:24 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 2, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

> On Oct. 1, 2015, 6:49 p.m., Timothy Chen wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, line 557
> > 
> >
> > Is this a bug fix? What was wrong? And why the tests pass before?

Its not a bug fix. GetManifest used to return layers in the order of fsLayers. 
But its now changed to match what store expects. The tests passed earlier 
becasue the code and tests were expecting the same order. Now that the order 
has been reversed in the code, the test had to be updated.


- Jojy


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


On Oct. 1, 2015, 6:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 1, 2015, 6:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 1, 2015, 6:39 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased with master


Repository: mesos


Description
---

- Moved ManifestResponse struct from RegistryClient to namespace.
- Cleanup


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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Timothy Chen]

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



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 520)


Add a comment why you go from the last to first



src/slave/containerizer/provisioner/docker/registry_client.cpp (line 591)


How about using the docker::Image::Name here?

We have repo, registry and tag already.


- Timothy Chen


On Oct. 1, 2015, 6:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 1, 2015, 6:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Ben Mahler]

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


Could you please split the changes here? Specifically it looks like pulling up 
the structs and the chagnes to control flow are independent.

- Ben Mahler


On Oct. 1, 2015, 6:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 1, 2015, 6:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 2, 2015, 12:24 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

review comments addressed.


Repository: mesos


Description
---

- Moved ManifestResponse struct from RegistryClient to namespace.
- Cleanup


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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

> On Oct. 1, 2015, 6:59 p.m., Ben Mahler wrote:
> > Could you please split the changes here? Specifically it looks like pulling 
> > up the structs and the chagnes to control flow are independent.

Done. Created https://reviews.apache.org/r/38941


- Jojy


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


On Oct. 1, 2015, 6:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> ---
> 
> (Updated Oct. 1, 2015, 6:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> 
> 
> Diffs
> -
> 
>   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/38579/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38579: Refactored registry client

[Jojy Varghese]

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

(Updated Oct. 1, 2015, 3:29 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

broke up big methods; cleanup.


Summary (updated)
-

Refactored registry client


Repository: mesos


Description (updated)
---

- Moved ManifestResponse struct from RegistryClient to namespace.
- Cleanup


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/38579/diff/


Testing
---

make check.


Thanks,

Jojy Varghese