|
|
Created:
12 years, 2 months ago by jcgregorio_google Modified:
12 years, 2 months ago CC:
google-api-python-client_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add comment on non-blocking streams. #
Total comments: 1
MessagesTotal messages: 11
Sign in to reply to this message.
I made one suggestion about making newly computed file sizes available to read after an upload, otherwise this looks good. http://codereview.appspot.com/6300093/diff/1/apiclient/http.py File apiclient/http.py (right): http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626 apiclient/http.py:626: size = str(self.resumable_progress + len(data)) It might be useful to update self.resumable._size here, so the value would be accessible outside of next_chunk().
Sign in to reply to this message.
http://codereview.appspot.com/6300093/diff/1/apiclient/http.py File apiclient/http.py (right): http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626 apiclient/http.py:626: size = str(self.resumable_progress + len(data)) I thought about that, but it seems wrong to alter a private property of an object that was passed into us. On 2012/06/14 15:14:46, yovadia wrote: > It might be useful to update self.resumable._size here, so the value would be > accessible outside of next_chunk().
Sign in to reply to this message.
In that case, maybe it should happen in MediaIoBaseUpload.getbytes() ? On Thu, Jun 14, 2012 at 8:41 AM, <jcgregorio@google.com> wrote: > > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py<http://cod... > File apiclient/http.py (right): > > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.** > py#newcode626<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626> > apiclient/http.py:626: size = str(self.resumable_progress + len(data)) > I thought about that, but it seems wrong to alter a private property of > an object that was passed into us. > > > On 2012/06/14 15:14:46, yovadia wrote: > >> It might be useful to update self.resumable._size here, so the value >> > would be > >> accessible outside of next_chunk(). >> > > http://codereview.appspot.com/**6300093/<http://codereview.appspot.com/6300093/> >
Sign in to reply to this message.
On 2012/06/14 15:49:50, yovadia wrote: > In that case, maybe it should happen in MediaIoBaseUpload.getbytes() ? Then all MediaUpload objects need to remember to change their _size during getbytes()? Seems like something that could be easily forgotten, and for not a lot of benefit. > > On Thu, Jun 14, 2012 at 8:41 AM, <mailto:jcgregorio@google.com> wrote: > > > > > > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py%3Chttp://c...> > > File apiclient/http.py (right): > > > > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.** > > > py#newcode626<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626> > > apiclient/http.py:626: size = str(self.resumable_progress + len(data)) > > I thought about that, but it seems wrong to alter a private property of > > an object that was passed into us. > > > > > > On 2012/06/14 15:14:46, yovadia wrote: > > > >> It might be useful to update self.resumable._size here, so the value > >> > > would be > > > >> accessible outside of next_chunk(). > >> > > > > > http://codereview.appspot.com/**6300093/%3Chttp://codereview.appspot.com/6300...> > >
Sign in to reply to this message.
http://codereview.appspot.com/6300093/diff/1/apiclient/http.py File apiclient/http.py (left): http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618 apiclient/http.py:618: self.resumable_progress, self.resumable.chunksize()) I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase files -- you can read and get fewer bytes back than you request and still not be at EOF. If you'd block, it returns None instead of 0. The only EOF is if you ask for n bytes and get 0. (So if you ask for chunksize, and get a part of chunksize, you have to ask again for the remainder and only if you get back zero are you at EOF.) http://docs.python.org/dev/library/io.html#io.RawIOBase.read
Sign in to reply to this message.
There are bigger problems if the stream is non-blocking, dribbling out what could be very small blocks of bytes would be very inefficient. If the stream is non-blocking then reads should be accumulated until they reach chunksize, and then sent. I will add a comment in the docstring that only blocking streams should be used. On 2012/06/14 19:54:53, nherring wrote: > http://codereview.appspot.com/6300093/diff/1/apiclient/http.py > File apiclient/http.py (left): > > http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618 > apiclient/http.py:618: self.resumable_progress, self.resumable.chunksize()) > I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase files -- > you can read and get fewer bytes back than you request and still not be at EOF. > If you'd block, it returns None instead of 0. The only EOF is if you ask for n > bytes and get 0. (So if you ask for chunksize, and get a part of chunksize, you > have to ask again for the remainder and only if you get back zero are you at > EOF.) > > http://docs.python.org/dev/library/io.html#io.RawIOBase.read
Sign in to reply to this message.
As things stand, I think you're right. I'd love to see HttpLib2 have real support for HTTP streams (i.e., not having the entire request in memory), and at that point, having an ability to put things on the wire as we get them would be great, so we're not stuck transmitting the whole chunksize when we get that last byte. :) On Thu, Jun 14, 2012 at 1:09 PM, <jcgregorio@google.com> wrote: > There are bigger problems if the stream is non-blocking, dribbling > out what could be very small blocks of bytes would be very > inefficient. If the stream is non-blocking then reads should be > accumulated until they reach chunksize, and then sent. > > I will add a comment in the docstring that only blocking streams > should be used. > > > > On 2012/06/14 19:54:53, nherring wrote: > >> http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py<http://cod... >> File apiclient/http.py (left): >> > > > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.** > py#oldcode618<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618> > >> apiclient/http.py:618: self.resumable_progress, >> > self.resumable.chunksize()) > >> I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase >> > files -- > >> you can read and get fewer bytes back than you request and still not >> > be at EOF. > >> If you'd block, it returns None instead of 0. The only EOF is if you >> > ask for n > >> bytes and get 0. (So if you ask for chunksize, and get a part of >> > chunksize, you > >> have to ask again for the remainder and only if you get back zero are >> > you at > >> EOF.) >> > > http://docs.python.org/dev/**library/io.html#io.RawIOBase.**read<http://docs.... >> > > > > http://codereview.appspot.com/**6300093/<http://codereview.appspot.com/6300093/> > -- Nathan Herring | Software Engineer | nherring@google.com | 616-466-4646
Sign in to reply to this message.
http://codereview.appspot.com/6300093/diff/1004/apiclient/http.py File apiclient/http.py (right): http://codereview.appspot.com/6300093/diff/1004/apiclient/http.py#newcode307 apiclient/http.py:307: opened in blocking mode, do not use streams opened in non-blocking mode. Updated comment on non-blocking streams.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2012/06/14 21:45:21, nherring wrote: > LGTM Committed in http://code.google.com/p/google-api-python-client/source/detail?r=3ba72a5faf1...
Sign in to reply to this message.
|