Add `chunk` segmented data library.

Add `chunk` segmented data library.

BUG=chromium:550673
Committed: https://github.com/luci/luci-go/commit/49ce9fadc532d4530dad90dc23fa0ef73bcd3590

[dnj, 2015-11-04 19:09:01]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, hinoka@chromium.org, iannucci@chromium.org

[dnj, 2015-11-04 19:09:01]

PTAL, split off from Bundler CL (https://codereview.chromium.org/1412063008).

[iannucci, 2015-11-05 01:10:08]

looks good to me. some comments.

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
File common/chunkstream/buffer.go (right):

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
common/chunkstream/buffer.go:14: // after the Consume() call is completed is an
error and may result in a panic.
So I think the following operations are mutually exclusive:
  * Append and Append
  * Append and Consume
  * Consume and Readers (Consume invalidates all Readers)

However, the following can happen concurrently:
  * Append and Multiple Readers
  * Multiple Readers

Right?

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
common/chunkstream/buffer.go:32: // Append adds another Chunk to the buffer.
another Chunk -> one or more Chunks

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
common/chunkstream/buffer.go:95: // If the limit is greater than zero, the
Reader's limit will be set.
I think this docstring is stale.

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
common/chunkstream/buffer.go:109: limit = b.size
hm.. why? what if you plan to Append stuff, since Append and Readers can
coexist?

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/b...
common/chunkstream/buffer.go:127: // from before a Consume call is an error and
will result in a panic.
nice! glad to see this is enforced with gen :)

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/c...
File common/chunkstream/chunk.go (right):

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/c...
common/chunkstream/chunk.go:12: type Chunk interface {
Is this intended to be used outside of the package? Is there a reason not to
have it be a *struct?

May be worth specifying that Bytes and Len must always return the SAME data.

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/c...
common/chunkstream/chunk.go:16: Len() int
maybe no Len() and then put Len() on chunkNode

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/r...
File common/chunkstream/reader.go (right):

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/r...
common/chunkstream/reader.go:24: type Reader struct {
rename to View

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/r...
common/chunkstream/reader.go:26: cidx     int
wat r this? comment pls?

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/r...
common/chunkstream/reader.go:84: func (r *Reader) Skip(count int64) int64 {
why return anything?

https://chromiumcodereview.appspot.com/1413923013/diff/1/common/chunkstream/r...
common/chunkstream/reader.go:124: func (r *Reader) indexDestructive(needle
[]byte) int64 {
What about making this public like `SkipTo(needle []byte) (skipped int64)` where
it skips to just-after the needle?

[dnj, 2015-11-13 23:22:04]

PTAL.

100% coverage BTW :)

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.go
File common/chunkstream/buffer.go (right):

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.g...
common/chunkstream/buffer.go:14: // after the Consume() call is completed is an
error and may result in a panic.
On 2015/11/05 01:10:07, iannucci wrote:
> So I think the following operations are mutually exclusive:
>   * Append and Append
>   * Append and Consume
>   * Consume and Readers (Consume invalidates all Readers)
> 
> However, the following can happen concurrently:
>   * Append and Multiple Readers
>   * Multiple Readers
> 
> Right?

Yep. I'll update the documentation to be more clear on this.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.g...
common/chunkstream/buffer.go:32: // Append adds another Chunk to the buffer.
On 2015/11/05 01:10:07, iannucci wrote:
> another Chunk -> one or more Chunks

Done.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.g...
common/chunkstream/buffer.go:95: // If the limit is greater than zero, the
Reader's limit will be set.
On 2015/11/05 01:10:07, iannucci wrote:
> I think this docstring is stale.

Done.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.g...
common/chunkstream/buffer.go:109: limit = b.size
On 2015/11/05 01:10:07, iannucci wrote:
> hm.. why? what if you plan to Append stuff, since Append and Readers can
> coexist?

Clearer with "View", but it's basically a snapshot.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/buffer.g...
common/chunkstream/buffer.go:127: // from before a Consume call is an error and
will result in a panic.
On 2015/11/05 01:10:07, iannucci wrote:
> nice! glad to see this is enforced with gen :)

Removed gen. :(

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/chunk.go
File common/chunkstream/chunk.go (right):

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/chunk.go...
common/chunkstream/chunk.go:12: type Chunk interface {
On 2015/11/05 01:10:08, iannucci wrote:
> Is this intended to be used outside of the package? Is there a reason not to
> have it be a *struct?
> 
> May be worth specifying that Bytes and Len must always return the SAME data.

I'm going to kill Len().

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/chunk.go...
common/chunkstream/chunk.go:16: Len() int
On 2015/11/05 01:10:07, iannucci wrote:
> maybe no Len() and then put Len() on chunkNode

Done.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/reader.go
File common/chunkstream/reader.go (right):

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/reader.g...
common/chunkstream/reader.go:24: type Reader struct {
On 2015/11/05 01:10:08, iannucci wrote:
> rename to View

Done.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/reader.g...
common/chunkstream/reader.go:26: cidx     int
On 2015/11/05 01:10:08, iannucci wrote:
> wat r this? comment pls?

Done.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/reader.g...
common/chunkstream/reader.go:84: func (r *Reader) Skip(count int64) int64 {
On 2015/11/05 01:10:08, iannucci wrote:
> why return anything?

No good reason.

https://codereview.chromium.org/1413923013/diff/1/common/chunkstream/reader.g...
common/chunkstream/reader.go:124: func (r *Reader) indexDestructive(needle
[]byte) int64 {
On 2015/11/05 01:10:08, iannucci wrote:
> What about making this public like `SkipTo(needle []byte) (skipped int64)`
where
> it skips to just-after the needle?

I did clean the internal interface up a bit, but I think this is not a good idea
as-is. Specifically, "indexDestructive" will mutate the buffer even if the
needle is not found, which is non-intuitive.

The modifications I made make it easy to add a "SkipTo" method if one is ever
desired, though.

[dnj, 2015-11-16 17:44:22]

Ping, wasn't clear if this lg'd t y.

[dnj, 2015-11-18 17:31:44]

Double-ping, if this is good I'd like to land it :)

[dnj (Google), 2015-11-18 23:08:33]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-11-18 23:08:33]

zzzping :( This is blocking another CL or two.

[iannucci, 2015-11-18 23:51:43]

lgtm

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
File common/chunkstream/buffer.go (right):

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
common/chunkstream/buffer.go:34: b.appendChunk(chunk)
maybe allocate a mini-list of chunks and then append it in one go? maybe not
necessary...

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
common/chunkstream/buffer.go:125: panic("consuming more data than available")
should we panic with errors? or is raw strings OK?

[dnj (Google), 2015-11-19 00:19:55]

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
File common/chunkstream/buffer.go (right):

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
common/chunkstream/buffer.go:34: b.appendChunk(chunk)
On 2015/11/18 23:51:42, iannucci wrote:
> maybe allocate a mini-list of chunks and then append it in one go? maybe not
> necessary...

Yeah it's not a bad idea, save some pointer updating. Then again, it adds a
memory allocation. Could special case 1 (common)... Meh, I'll leave as-is.

https://codereview.chromium.org/1413923013/diff/20001/common/chunkstream/buff...
common/chunkstream/buffer.go:125: panic("consuming more data than available")
On 2015/11/18 23:51:43, iannucci wrote:
> should we panic with errors? or is raw strings OK?

TBH I'm not sure. I don't think an error would add anything to the picture, but
I suppose might as well.

[dnj (Google), 2015-11-19 00:21:09]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2015-11-19 00:21:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1413923013/#ps40001
(title: "Use errors in pacnic.")

[commit-bot: I haz the power, 2015-11-19 00:21:18]

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

[commit-bot: I haz the power, 2015-11-19 00:23:31]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/49ce9fadc532d4530dad90dc23fa0ef73bcd3590