api: use simple streaming upload for GetObject() reader.#735
Conversation
harshavardhana
force-pushed
the
get-object
branch
from
0b13838 to
5d12f90
Compare
|
PR is updated fixes a bug as well in the streaming signature implementation. Can anyone review @krishnasrinivas @vadmeste ? |
| for { | ||
| n1, err := s.baseReadCloser.Read(s.chunkBuf[s.chunkBufLen:]) | ||
| if err == nil || err == io.ErrUnexpectedEOF { | ||
| // An instance of this general case is that a Reader returning |
There was a problem hiding this comment.
We could simplify this comment like,
1. n > 0, err is one of io.EOF, io.ErrUnexpectedEOF, nil - nearing end of stream
2. n ==0, err is io.EOF - actual end of stream
5d12f90 to
211405c
Compare
harshavardhana
force-pushed
the
get-object
branch
2 times, most recently
from
ab67b56 to
d9ba0cc
Compare
| // Usually we validate `err` first, but in this case | ||
| // we are validating n > 0 for the following reasons. | ||
| // | ||
| // 1. n > 0, err is one of io.EOF, io.ErrUnexpectedEOF, nil |
There was a problem hiding this comment.
But Read() would not return io.ErrUnexpectedEOF, right?
There was a problem hiding this comment.
Actually you are right it either returns io.EOF or nil. Let me change it.
| // It is to indicate that *minio.Object implements io.ReaderAt. | ||
| // and such a functionality is used in the subsequent code path. | ||
| if isObject(reader) || isReadAt(reader) || isFile(reader) { | ||
| if isFile(reader) || !isObject(reader) && isReadAt(reader) { |
There was a problem hiding this comment.
The intention is to disable parallel upload of the parts if the src is minio.Object?
There was a problem hiding this comment.
That is correct.. since it is not possible to support streaming parallel downloads in current structure for GetObject to PutObject(). It can lead to some complications in the code base.. so it is pushed as a future work to get back to.
There was a problem hiding this comment.
Instead we will add concurrent copy code on the mc side for in multiples of files.
There was a problem hiding this comment.
There is a possibility by implementing io.SectionReader() based on io.ReadSeeker instead of io.ReaderAt. But it is not worth the complexity for now in my opinion.
| // We do not have to upload till totalPartsCount. | ||
| if err == io.EOF && size < 0 { | ||
| break | ||
| } |
There was a problem hiding this comment.
Does this mean we will not be supporting upload if size is -1?
There was a problem hiding this comment.
Streaming we never support size == -1, for size == -1 we fall back to regular multipart upload, which never reaches here..
harshavardhana
force-pushed
the
get-object
branch
from
d9ba0cc to
fe70504
Compare
| break | ||
| } | ||
| } | ||
| if err != nil && err != io.ErrUnexpectedEOF { |
There was a problem hiding this comment.
@harshavardhana there is a reference to io.ErrUnexpectedEOF here as well
harshavardhana
force-pushed
the
get-object
branch
2 times, most recently
from
a445711 to
160c5ea
Compare
This change is needed to avoid the offset based ReadAt() calls which can occur if using io.SectionReader(). Optimizing this part re-implementing a sectionReader() based on Seeker() wasn't worth the complexity. This PR also fixes the bug where the streaming signature was interpreting the Read() error return in a wrong manner.
harshavardhana
force-pushed
the
get-object
branch
from
160c5ea to
77cdda9
Compare
|
@krishnasrinivas any more reviews ? |
3 participants
This change is needed to avoid the offset based ReadAt()
calls which can occur if using io.SectionReader().
Optimizing this part re-implementing a sectionReader()
based on Seeker() wasn't worth the complexity.