-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WAL directory tailer #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, this is looking good. Good question wrt. API, I think for now simple is good.
tail/tail.go
Outdated
type tailer struct { | ||
ctx context.Context | ||
dir string | ||
next int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you call this nextNoun, for documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "noun"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant as a placeholder. I'm not sure what noun you're tracking there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes – changed it to nextSegment
.
// Tail the prommetheus/tsdb write ahead log in the given directory. Checkpoints | ||
// are read before reading any WAL segments. | ||
// Tailing may fail if we are racing with the DB itself in deleting obsolete checkpoints | ||
// and segments. The caller should implement relevant logic to retry in those cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the relevant logic to call this method again? Is that the only failure mode, and if not, should we add a retriable error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a special error. Many things could fail of course – anything along file opens/reads/etc. It's hard (and probably sometimes impossible) to tell which ones a permanent failures.
I'd just go with the caller retrying with a backoff strategy. Since none of this writes data, we shouldn't be messing anything up.
@@ -0,0 +1,112 @@ | |||
package tail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the Google license.
tail/tail.go
Outdated
if err == tsdb.ErrNotFound { | ||
// Next segment doesn't exist yet. We'll probably just have to | ||
// wait for more data to be written. | ||
time.Sleep(time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to pass backoff instead of time.Second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! :)
Signed-off-by: Fabian Reinartz <freinartz@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jkohen this should do the job but let me know whether we need some additional insight like current
(segment, offset)
or similar.