Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Nov. 6, 2015, 3:50 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

reviews


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

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



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


Let's not ignore the error for os::close().

Please log when you cannot close the fd and the error.



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


Also please include that the future is discarded the failure message.


- Timothy Chen


On Nov. 6, 2015, 3:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 3:50 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Nov. 6, 2015, 5:15 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

review


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

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



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


I think I'll format this differently, but I can fix that myself.


- Timothy Chen


On Nov. 6, 2015, 5:15 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 5:15 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

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


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

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


Review request for mesos, Ben Mahler and Timothy Chen.


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Nov. 5, 2015, 11:58 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

review


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Nov. 6, 2015, 12:38 a.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Changes
---

removed shared_ptr based count


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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



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


For manifest case i didnt think we needed streaming.


- Jojy Varghese


On Nov. 5, 2015, 7:18 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 5, 2015, 7:18 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

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



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


When does no streaming makes sense? I wonder if we should just use 
streaming as the only option?



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


why _writeBlob? It's not a continuation from writeBlob right?
It should be _saveBlob if it's a continuation



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


Why don't we just copy on each call? Seems unncessary to have a shared_ptr 
int especially only one save in flight at a time.



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


Should we log when we failed to close?



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


onAny doesn't return anything.
We try to propgage the error back to the caller so they log themselves, 
unless you have more information here to log.



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


We usually say it's discarded in the message too.


- Timothy Chen


On Nov. 5, 2015, 7:18 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 5, 2015, 7:18 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

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



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


But we're not modifying totalSize at all, only use it to create a new 
variable on the stack.

Let me try it locally.


- Timothy Chen


On Nov. 6, 2015, 12:38 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

> On Nov. 5, 2015, 8:37 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line 
> > 92
> > 
> >
> > When does no streaming makes sense? I wonder if we should just use 
> > streaming as the only option?

Dropping this as we discussed it.


- Jojy


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


On Nov. 6, 2015, 12:38 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

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



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


How about:
size_t newTotalSize = data.length() + totalSize;

 return _saveBlob(fd, data.data(), data.length())
.then([this, fd, reader, newSize](
)  -> Future {

  return saveBlob(fd, const_cast(reader), 
newTotalSize);
});

Then you shouldn't need any mutable marks right?


- Timothy Chen


On Nov. 6, 2015, 12:38 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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



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


We still need mutable in the wrapping lambda since we are capturing 
totalSize inside that too.


- Jojy Varghese


On Nov. 6, 2015, 12:38 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Timothy Chen]

> On Nov. 6, 2015, 4:56 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp, line 
> > 637
> > 
> >
> > But we're not modifying totalSize at all, only use it to create a new 
> > variable on the stack.
> > 
> > Let me try it locally.

Ok just confirmed this works: 
https://gist.github.com/tnachen/990b1b0bb0c9e7f8d23c
No need for mutable.


- Timothy


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


On Nov. 6, 2015, 12:38 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Nov. 6, 2015, 12:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Nov. 4, 2015, 11:23 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

rebased.


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

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


Review request for mesos and Ben Mahler.


Changes
---

rebased.


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

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


Review request for mesos and Ben Mahler.


Changes
---

removed defers


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Oct. 23, 2015, 4:55 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Fixed offset based write


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

(Updated Oct. 16, 2015, 12:15 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Fixed io::write to use the correct write overload.


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39013, 38443, 39184, 39155, 39156, 38579, 39014, 39068, 
38941, 39015, 39016, 39017, 39053, 39112, 39340]

All tests passed.

- Mesos ReviewBot


On Oct. 16, 2015, 12:15 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Oct. 16, 2015, 12:15 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> 471783d88b73b62afacac3d7952ebb5d5f442097 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39340: RegistryClient: Added streaming response read

[Jojy Varghese]

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

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


Review request for mesos and Ben Mahler.


Changes
---

Fixed total read count


Repository: mesos


Description
---

RegistryClient: Added streaming response read


Diffs (updated)
-

  src/slave/containerizer/provisioner/docker/registry_client.cpp 
471783d88b73b62afacac3d7952ebb5d5f442097 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39340: RegistryClient: Added streaming response read

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39013, 38443, 39184, 39155, 39156, 38579, 39014, 39068, 
38941, 39015, 39016, 39017, 39053, 39112, 39340]

All tests passed.

- Mesos ReviewBot


On Oct. 15, 2015, 4:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39340/
> ---
> 
> (Updated Oct. 15, 2015, 4:03 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> RegistryClient: Added streaming response read
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> 471783d88b73b62afacac3d7952ebb5d5f442097 
> 
> Diff: https://reviews.apache.org/r/39340/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>