Fix the limit for the debug-log calls.

Fix the limit for the debug-log calls.

Unbeknownst to me, the filter method was being called
during the backlog iteration as well.  This fix introduces
a callback that the tailer calls when it starts the forward
filtering.

This allows us to count the filter calls only when the
filter is being called to write out the results. We cannot
count the write calls because the tailer uses buffered i/o
there.

https://code.launchpad.net/~thumper/juju-core/fix-backlog-limits/+merge/215333

Requires: https://code.launchpad.net/~thumper/juju-core/debug-log-api/+merge/215323

(do not edit description out of merge proposal)

[thumper, 2014-04-11 02:11:13]

Please take a look.

[axw, 2014-04-11 08:07:45]

https://codereview.appspot.com/85570045/diff/1/utils/tailer/tailer.go
File utils/tailer/tailer.go (right):

https://codereview.appspot.com/85570045/diff/1/utils/tailer/tailer.go#newcode56
utils/tailer/tailer.go:56: filter TailerFilterFunc, callback
TailerFilterStartedFunc) *Tailer {
Rather than adding *another* parameter that's only relevant to backtracking, I
really think it would be best to ditch NewTailerBacktrack and create a separate
function that does the backtracking.

https://codereview.appspot.com/85570045/diff/1/utils/tailer/tailer.go#newcode65
utils/tailer/tailer.go:65: filter TailerFilterFunc, callback
TailerFilterStartedFunc) *Tailer {
Isn't having a callback fairly pointless here? It's going to be called
immediately.

[rog, 2014-04-11 08:20:14]

As far as I can see, this makes a nice Tailer API into
a not-so-nice one because you've made a stateful
filter function and you want it to be called in just
the way you expect.

I don't think the filter function should be stateful.

I can see a couple of solutions that might be better:

1) (my preferred option) ditch the server-side maxLines
functionality completely. It could be implemented client-side by
simply closing the connection when the right number
of lines is received, but we've always got head(1),
so I'd suggest just losing it entirely.
2) add maxLines functionality to the Tailer itself.

1) might use a little extra bandwidth,
but I can't see that that would be great problem
in practice. If it is, the second option could
be implemented later.

[thumper, 2014-04-14 01:14:00]

Due to the buffered i/o in the tailer, I'd prefer to keep the line parsing
functionality in the server side.

However I think that axw's approach of breaking out the backtracking is a good
one, and that is the approach I'll use.

[thumper, 2014-04-14 02:10:45]

Please take a look.

[axw, 2014-04-14 03:52:39]

LGTM with a few suggestions

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.go
File state/apiserver/debuglog.go (right):

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.g...
state/apiserver/debuglog.go:225: err := tailer.SeekLastLines(logFile,
stream.backlog, stream.filterLine)
return tailer.SeekLastLines(...

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.g...
state/apiserver/debuglog.go:258: if stream.started && result && stream.maxLines
> 0 {
It would be better to just split filterLine into something non-counting and
counting, where the latter calls the former and adds the linecount check. Then
you don't need the boolean.

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

https://codereview.appspot.com/85570045/diff/20001/utils/tailer/tailer.go#new...
utils/tailer/tailer.go:31: // TailerFilterStartedFunc is a callback that is
called when the filtering is
Delete this?

[rog, 2014-04-14 07:11:09]

> Due to the buffered i/o in the tailer, I'd prefer to keep the line parsing
> functionality in the server side.

I don't quite understand this remark (the buffer is flushed whenever we get to
the end of the file, so it shouldn't make any significant difference), but
splitting out the backtracking code makes me much happier.

LGTM with a couple of trivial suggestions below.

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.go
File state/apiserver/debuglog.go (right):

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.g...
state/apiserver/debuglog.go:224: if !stream.fromTheStart {
if stream.fromTheStart {
    return nil
}
...

saves a negative and a level of indentation.

https://codereview.appspot.com/85570045/diff/20001/state/apiserver/debuglog.g...
state/apiserver/debuglog.go:258: if stream.started && result && stream.maxLines
> 0 {
On 2014/04/14 03:52:40, axw wrote:
> It would be better to just split filterLine into something non-counting and
> counting, where the latter calls the former and adds the linecount check. Then
> you don't need the boolean.

+1

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

https://codereview.appspot.com/85570045/diff/20001/utils/tailer/tailer.go#new...
utils/tailer/tailer.go:31: // TailerFilterStartedFunc is a callback that is
called when the filtering is
On 2014/04/14 03:52:40, axw wrote:
> Delete this?

+1

https://codereview.appspot.com/85570045/diff/20001/utils/tailer/tailer.go#new...
utils/tailer/tailer.go:132: // wanted number of filtered lines before the end.
// If filter is non-nil, only lines for which filter returns
// true will be counted.

?