Please take a look.


https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go
File utils/tailer.go (right):

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode30
utils/tailer.go:30: func StartFileTailer(filename string, lines int,
filter TailerFilterFunc,
On 2013/12/05 16:20:37, rog wrote:
> I think this function is unnecessary. It's trivial for other code to
open a
> file.

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode40
utils/tailer.go:40: func StartTailer(readSeeker io.ReadSeeker, lines
int, filter TailerFilterFunc,
On 2013/12/05 16:20:37, rog wrote:
> Please document this function properly.

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode87
utils/tailer.go:87: println("> error:", err.Error())
On 2013/12/05 16:20:37, rog wrote:
> d

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode98
utils/tailer.go:98: if buffer[i] == '\n' {
On 2013/12/05 16:20:37, rog wrote:
> This isn't strictly accurate (what happens when we have an
unterminated line at
> the end of the file?) but it's probably ok if we never filter
unterminated
> lines. A comment about why it's ok might prevent future puzzling over
the
> correctness of the code.

Added a comment at the type declaration.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode100
utils/tailer.go:100: if foundNewlines-1 == t.lines {
On 2013/12/05 16:20:37, rog wrote:
> s/==/>=/

> defensively.

> Also, if we move the if statement before the increment,
> then you can lose the "-1".

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode127
utils/tailer.go:127: line, err := reader.ReadString('\n')
On 2013/12/05 16:20:37, rog wrote:
> I think I'd be tempted to use ReadSlice here (changing the type of
filter to
> func([]byte)bool.
> That way the tailer won't need to generate any garbage at all AFAICS.

> Unfortunately that limits the line size to the size of the bufio
buffer, which
> may be acceptable. An alternative is to use ReadLine and allocate only
if the
> line is too long.

> Here's some code I wrote a while ago to do that, in case it might be
useful:

> // readLine reads a line from r.
> // The returned byte slice is only valid until the
> // next read call on r.
> func readLine(r *bufio.Reader) ([]byte, error) {
> 	line, isPrefix, err := r.ReadLine()
> 	if !isPrefix {
> 		return line, err
> 	}
> 	buf := append([]byte(nil), line...)
> 	for isPrefix && err == nil {
> 		line, isPrefix, err = r.ReadLine()
> 		buf = append(buf, line...)
> 	}
> 	return buf, err
> }

Good hint, using it.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode147
utils/tailer.go:147: for {
On 2013/12/05 16:20:37, rog wrote:
> This code is exactly the same as the code above.
> Can't we do with just this? (if we just delete the
> loop above and use NewTimer(0), I think it might
> just work).

Yep, the split has been due to the initial approach of scanning all up
to the end first. Removed. Thanks.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode149
utils/tailer.go:149: if len(line) > 0 {
On 2013/12/05 16:20:37, rog wrote:
> I think we should ignore the line at this point if it is unterminated,
something
> that's quite possible if the file is being written in arbitrary
chunks. If we
> find an unterminated line, we should seek back to the start of the
line before
> we start reading again.

> That means we could pass the line to filter without the trailing \n,
meaning we
> can be more resilient if something starts producing lines ending in
\r\n.

Found a solution for unterminated lines, but currently ignoring possible
\r\n and passing the termination to the filter too. For the work with
other delimiters than \n more changes are needed.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode151
utils/tailer.go:151: writer.WriteString(line)
On 2013/12/05 16:20:37, rog wrote:
> We should definitely check the error here and in Flush below - I don't
think we
> want to carry on trying to write to a dead network connection or full
> filesystem.

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer.go#newcode162
utils/tailer.go:162: t.readSeeker.Seek(0, os.SEEK_END)
On 2013/12/05 16:20:37, rog wrote:
> This is wrong, I think - we could miss data if some has been
> written in between seeing the EOF and this seek.
> I think the only time we want to seek is if we get an unterminated
line.

> That said, there's another tricky case - what happens if the file gets
> truncated? I wonder if that should be done with separate logic to
detect whether
> the file has suddenly reduced in size. We'll need separate logic
anyway to
> decide when to reopen the file when log files roll over, so that might
work out
> ok.

Removed the seek and keeping the truncation in mind.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go
File utils/tailer_test.go (right):

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go#newcode25
utils/tailer_test.go:25: buffer := bytes.NewBuffer([]byte{})
On 2013/12/05 16:20:37, rog wrote:
> var buffer bytes.Buffer

> is simpler and does almost the same thing.

Almost, but I need the pointer (for io.Writer in NewTailer() and in the
assertion).

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go#newcode124
utils/tailer_test.go:124: func assertCollected(c *gc.C, buffer
*bytes.Buffer, collected []string, addon func([]string), timeout bool) {
On 2013/12/05 16:20:37, rog wrote:
> Please could we have a comment describing what this function is doing,
please?

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go#newcode162
utils/tailer_test.go:162: var rs *readSeeker = new(readSeeker)
On 2013/12/05 16:20:37, rog wrote:
> var rs readSeeker

Done.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go#newcode206
utils/tailer_test.go:206: if r.pos == len(r.buffer) {
On 2013/12/05 16:20:37, rog wrote:
> I think we can usefully use the result of copy here:

> if r.pos >= len(r.buffer) {
>     return 0, io.EOF
> }

> n := copy(p, r.buffer[r.pos:])
> r.pos += n
> return n, nil

Awesome, totally forgot the return value of copy. *sigh* Have to get
better regarding slice operations.

https://codereview.appspot.com/36540043/diff/20001/utils/tailer_test.go#newcode244
utils/tailer_test.go:244: func signal(c *gc.C, sigc chan struct{}) {
On 2013/12/05 16:20:37, rog wrote:
> Given that the signal channel is buffered, can't we just send on the
channel?

Oh, yes, that's left from an older approach.

https://codereview.appspot.com/36540043/