LogDog: Fix archived log stream read errors.

LogDog: Fix archived log stream read errors.

Fix some bugs in LogDog and associated libraries that prevented reading
very large Google Storage archived streams.

Fundamentally, Google Storage reads were not upper-bounded, despite the
fact that we had the information to do so. This caused (minimally) more
data than necessary to be fetched for a given request, and (in this
case) a request to be issued that exceeded GAE's `urlfetch` upper bound,
32MB. We fix this by updating the archive log storage instance to
calculate an upper offset when requesting log stream ranges.

This exposed a bug in Google Storage library, where "from" and "to"
parameters (absolute byte offsets) were being translated to "offset" and
"length" parameters (with "length" relative to "offset"). This is fixed
by explicitly standardizing on offset/length.

Some `logs.Get` endpoint tests constrain byte count. Because we're now
specifying upper bound, this caused them to return fewer logs than
expected because the byte count being applied didn't include the RecordIO
frame size. We fix this by calculating the frame size and factoring that
into the byte range requests.

BUG=chromium:604610
TEST=live
  - Deployed to afflicted system, verified that log stream is now
    functional.

Committed: https://github.com/luci/luci-go/commit/1ec9fb582858f2063f64030370c9912b518726f6

[dnj, 2016-04-19 21:54:49]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, nodir@chromium.org, vadimsh@chromium.org

[dnj, 2016-04-19 21:54:49]

+vadimsh@, nodir@, could one of you please review?

+estaab@ FYI, since it's your bug :)

[nodir, 2016-04-19 23:41:17]

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go
File common/gcloud/gs/gs.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:42: // is <0, no upper byte bound will be set.
why not accept uint64 or panic? if someone passes a negative value, it sounds
like a bug in the caller code.
if you use uint64, you wouldn't have to do type conversion in client code

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:43: NewReader(p Path, offset int64, length int64)
(io.ReadCloser, error)
nit: offset, length int64

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:109: func (c *prodClient) NewReader(p Path, offset int64,
length int64) (io.ReadCloser, error) {
nit: offset, length int64

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.go
File common/recordio/size_test.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.g...
common/recordio/size_test.go:18: Convey(`Testing FrameHeaderSize`, t, func() {
I believe this test must use binary.PutUvarint

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.g...
common/recordio/size_test.go:44: type devNull struct{}
Use ioutil.Discard

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
st.offset()?
inline it to be consistent with length

[dnj, 2016-04-20 00:45:07]

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go
File common/gcloud/gs/gs.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:42: // is <0, no upper byte bound will be set.
On 2016/04/19 23:41:16, nodir wrote:
> why not accept uint64 or panic? if someone passes a negative value, it sounds
> like a bug in the caller code.
> if you use uint64, you wouldn't have to do type conversion in client code

We generally don't do uint64 in our code, even for positive-only values. There
was a long discussion in the past about this, so I'll omit the specifics. Other
APIs (including Cloud Storage) also generally don't use uint64.

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:43: NewReader(p Path, offset int64, length int64)
(io.ReadCloser, error)
On 2016/04/19 23:41:16, nodir wrote:
> nit: offset, length int64

Done.

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:109: func (c *prodClient) NewReader(p Path, offset int64,
length int64) (io.ReadCloser, error) {
On 2016/04/19 23:41:16, nodir wrote:
> nit: offset, length int64

Done.

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.go
File common/recordio/size_test.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.g...
common/recordio/size_test.go:18: Convey(`Testing FrameHeaderSize`, t, func() {
On 2016/04/19 23:41:16, nodir wrote:
> I believe this test must use binary.PutUvarint

I'm not sure what you mean by "must use"? It indirectly uses it via
"WriteFrame", which calls that, but since I'm measuring *frame* length,
implementing parts of WriteFrame for the test is less useful than specifically
writing a frame and comparing the results.

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.g...
common/recordio/size_test.go:44: type devNull struct{}
On 2016/04/19 23:41:17, nodir wrote:
> Use ioutil.Discard

Done.

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/19 23:41:17, nodir wrote:
> st.offset()?
> inline it to be consistent with length

length is calculated, offset is not. There's no reason to make a function out of
it.

[nodir, 2016-04-20 16:42:57]

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go
File common/gcloud/gs/gs.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:42: // is <0, no upper byte bound will be set.
On 2016/04/20 00:45:07, dnj wrote:
> On 2016/04/19 23:41:16, nodir wrote:
> > why not accept uint64 or panic? if someone passes a negative value, it
sounds
> > like a bug in the caller code.
> > if you use uint64, you wouldn't have to do type conversion in client code
> 
> We generally don't do uint64 in our code, even for positive-only values. There
> was a long discussion in the past about this, so I'll omit the specifics.
Other
> APIs (including Cloud Storage) also generally don't use uint64.

ok, why not panic? If there is a negative value, it is a bug in your code. Don't
you want to find bugs?

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.go
File common/recordio/size_test.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/recordio/size_test.g...
common/recordio/size_test.go:18: Convey(`Testing FrameHeaderSize`, t, func() {
On 2016/04/20 00:45:07, dnj wrote:
> On 2016/04/19 23:41:16, nodir wrote:
> > I believe this test must use binary.PutUvarint
> 
> I'm not sure what you mean by "must use"? It indirectly uses it via
> "WriteFrame", which calls that, but since I'm measuring *frame* length,
> implementing parts of WriteFrame for the test is less useful than specifically
> writing a frame and comparing the results.

yeah indeed

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/20 00:45:07, dnj wrote:
> On 2016/04/19 23:41:17, nodir wrote:
> > st.offset()?
> > inline it to be consistent with length
> 
> length is calculated, offset is not. There's no reason to make a function out
of
> it.

make a function? It is already made. Otherwise why it was made?

[dnj, 2016-04-20 17:07:25]

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go
File common/gcloud/gs/gs.go (right):

https://codereview.chromium.org/1904503003/diff/1/common/gcloud/gs/gs.go#newc...
common/gcloud/gs/gs.go:42: // is <0, no upper byte bound will be set.
On 2016/04/20 16:42:57, nodir wrote:
> On 2016/04/20 00:45:07, dnj wrote:
> > On 2016/04/19 23:41:16, nodir wrote:
> > > why not accept uint64 or panic? if someone passes a negative value, it
> sounds
> > > like a bug in the caller code.
> > > if you use uint64, you wouldn't have to do type conversion in client code
> > 
> > We generally don't do uint64 in our code, even for positive-only values.
There
> > was a long discussion in the past about this, so I'll omit the specifics.
> Other
> > APIs (including Cloud Storage) also generally don't use uint64.
> 
> ok, why not panic? If there is a negative value, it is a bug in your code.
Don't
> you want to find bugs?

Sure, I'll panic if offset is <0.

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/20 16:42:57, nodir wrote:
> On 2016/04/20 00:45:07, dnj wrote:
> > On 2016/04/19 23:41:17, nodir wrote:
> > > st.offset()?
> > > inline it to be consistent with length
> > 
> > length is calculated, offset is not. There's no reason to make a function
out
> of
> > it.
> 
> make a function? It is already made. Otherwise why it was made?

st.startOffset is not a struct member variable, not a function. If I create
"st.offset()" like you suggested, I will be creating a new function, which I
don't want to do

[dnj, 2016-04-20 17:08:08]

Description was changed from

==========
LogDog: Fix archived log stream read errors.

Fix some bugs in LogDog and associated libraries that prevented reading
very large Google Storage archived streams.

Fundamentally, Google Storage reads were not upper-bounded, despite the
fact that we had the information to do so. This caused (minimally) more
data than necessary to be fetched for a given request, and (in this
case) a request to be issued that exceeded GAE's `urlfetch` upper bound,
32MB. We fix this by updating the archive log storage instance to
calculate an upper offset when requesting log stream ranges.

This exposed a bug in Google Storage library, where "from" and "to"
parameters (absolute byte offsets) were being translated to "offset" and
"length" parameters (with "length" relative to "offset"). This is fixed
by explicitly standardizing on offset/length.

Some `logs.Get` endpoint tests constrain byte count. Because we're now
specifying upper bound, this caused them to return fewer logs than
expected because the byte count being applied didn't include the RecordIO
frame size. We fix this by calculating the frame size and factoring that
into the byte range requests.

BUG=chromium:604610
TEST=live
  - Deployed to afflicted system, verified that log stream is not
    functional.
==========

to

==========
LogDog: Fix archived log stream read errors.

Fix some bugs in LogDog and associated libraries that prevented reading
very large Google Storage archived streams.

Fundamentally, Google Storage reads were not upper-bounded, despite the
fact that we had the information to do so. This caused (minimally) more
data than necessary to be fetched for a given request, and (in this
case) a request to be issued that exceeded GAE's `urlfetch` upper bound,
32MB. We fix this by updating the archive log storage instance to
calculate an upper offset when requesting log stream ranges.

This exposed a bug in Google Storage library, where "from" and "to"
parameters (absolute byte offsets) were being translated to "offset" and
"length" parameters (with "length" relative to "offset"). This is fixed
by explicitly standardizing on offset/length.

Some `logs.Get` endpoint tests constrain byte count. Because we're now
specifying upper bound, this caused them to return fewer logs than
expected because the byte count being applied didn't include the RecordIO
frame size. We fix this by calculating the frame size and factoring that
into the byte range requests.

BUG=chromium:604610
TEST=live
  - Deployed to afflicted system, verified that log stream is now
    functional.
==========

[dnj, 2016-04-20 17:08:41]

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/20 17:07:25, dnj wrote:
> On 2016/04/20 16:42:57, nodir wrote:
> > On 2016/04/20 00:45:07, dnj wrote:
> > > On 2016/04/19 23:41:17, nodir wrote:
> > > > st.offset()?
> > > > inline it to be consistent with length
> > > 
> > > length is calculated, offset is not. There's no reason to make a function
> out
> > of
> > > it.
> > 
> > make a function? It is already made. Otherwise why it was made?
> 
> st.startOffset is not a struct member variable, not a function. If I create
> "st.offset()" like you suggested, I will be creating a new function, which I
> don't want to do

This should read, "st.startOffset *is* a struct member variable".

[nodir, 2016-04-20 17:09:48]

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/20 17:07:25, dnj wrote:
> On 2016/04/20 16:42:57, nodir wrote:
> > On 2016/04/20 00:45:07, dnj wrote:
> > > On 2016/04/19 23:41:17, nodir wrote:
> > > > st.offset()?
> > > > inline it to be consistent with length
> > > 
> > > length is calculated, offset is not. There's no reason to make a function
> out
> > of
> > > it.
> > 
> > make a function? It is already made. Otherwise why it was made?
> 
> st.startOffset is not a struct member variable, not a function. If I create
> "st.offset()" like you suggested, I will be creating a new function, which I
> don't want to do

st variable is of type *getStrategy
*getStrategy has offset method

[dnj, 2016-04-20 17:16:14]

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
File server/logdog/storage/archive/storage.go (right):

https://codereview.chromium.org/1904503003/diff/1/server/logdog/storage/archi...
server/logdog/storage/archive/storage.go:122: offset := int64(st.startOffset)
On 2016/04/20 17:09:48, nodir wrote:
> On 2016/04/20 17:07:25, dnj wrote:
> > On 2016/04/20 16:42:57, nodir wrote:
> > > On 2016/04/20 00:45:07, dnj wrote:
> > > > On 2016/04/19 23:41:17, nodir wrote:
> > > > > st.offset()?
> > > > > inline it to be consistent with length
> > > > 
> > > > length is calculated, offset is not. There's no reason to make a
function
> > out
> > > of
> > > > it.
> > > 
> > > make a function? It is already made. Otherwise why it was made?
> > 
> > st.startOffset is not a struct member variable, not a function. If I create
> > "st.offset()" like you suggested, I will be creating a new function, which I
> > don't want to do
> 
> st variable is of type *getStrategy
> *getStrategy has offset method

Ah I see, I didn't realize that was there. This is an artifact of a previous
approach, I'm going to delete it.

[nodir, 2016-04-20 17:17:17]

lgtm

[dnj, 2016-04-20 19:30:18]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-04-20 19:30:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904503003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904503003/60001

[commit-bot: I haz the power, 2016-04-20 19:35:07]

Description was changed from

==========
LogDog: Fix archived log stream read errors.

Fix some bugs in LogDog and associated libraries that prevented reading
very large Google Storage archived streams.

Fundamentally, Google Storage reads were not upper-bounded, despite the
fact that we had the information to do so. This caused (minimally) more
data than necessary to be fetched for a given request, and (in this
case) a request to be issued that exceeded GAE's `urlfetch` upper bound,
32MB. We fix this by updating the archive log storage instance to
calculate an upper offset when requesting log stream ranges.

This exposed a bug in Google Storage library, where "from" and "to"
parameters (absolute byte offsets) were being translated to "offset" and
"length" parameters (with "length" relative to "offset"). This is fixed
by explicitly standardizing on offset/length.

Some `logs.Get` endpoint tests constrain byte count. Because we're now
specifying upper bound, this caused them to return fewer logs than
expected because the byte count being applied didn't include the RecordIO
frame size. We fix this by calculating the frame size and factoring that
into the byte range requests.

BUG=chromium:604610
TEST=live
  - Deployed to afflicted system, verified that log stream is now
    functional.
==========

to

==========
LogDog: Fix archived log stream read errors.

Fix some bugs in LogDog and associated libraries that prevented reading
very large Google Storage archived streams.

Fundamentally, Google Storage reads were not upper-bounded, despite the
fact that we had the information to do so. This caused (minimally) more
data than necessary to be fetched for a given request, and (in this
case) a request to be issued that exceeded GAE's `urlfetch` upper bound,
32MB. We fix this by updating the archive log storage instance to
calculate an upper offset when requesting log stream ranges.

This exposed a bug in Google Storage library, where "from" and "to"
parameters (absolute byte offsets) were being translated to "offset" and
"length" parameters (with "length" relative to "offset"). This is fixed
by explicitly standardizing on offset/length.

Some `logs.Get` endpoint tests constrain byte count. Because we're now
specifying upper bound, this caused them to return fewer logs than
expected because the byte count being applied didn't include the RecordIO
frame size. We fix this by calculating the frame size and factoring that
into the byte range requests.

BUG=chromium:604610
TEST=live
  - Deployed to afflicted system, verified that log stream is now
    functional.

Committed:
https://github.com/luci/luci-go/commit/1ec9fb582858f2063f64030370c9912b518726f6
==========

[commit-bot: I haz the power, 2016-04-20 19:35:08]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/1ec9fb582858f2063f64030370c9912b518726f6