feat(journald source): Add checkpointing support #816
Conversation
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
|
Nice work. I appreciate that you split out the file |
| # | ||
| # * optional | ||
| # * no default | ||
| data_dir = "/var/lib/vector" |
There was a problem hiding this comment.
this sounds like something we should add to the file source too since it has check pointing as well?
There was a problem hiding this comment.
I got this from the file source. I'm not sure what it is we should add to it then.
lib/journald/src/lib.rs
Outdated
| Ok(into_string(cursor).unwrap()) | ||
| } | ||
|
|
||
| pub fn seek_cursor(&self, cursor: &str) -> IOResult<()> { |
There was a problem hiding this comment.
seems odd that we are passing around a str for a cursor is there any more type level stuff we could add to this?
There was a problem hiding this comment.
The cursor from libsystemd is a string. Do you mean to wrap it in a dummy struct?
| @@ -124,21 +137,54 @@ fn create_event(record: Record) -> Event { | |||
| log.into() | |||
| } | |||
|
|
|||
| trait JournalCursor { | |||
There was a problem hiding this comment.
looks like below you bound this with Iterator<Item = Result<Record, Error>> would it make sense to bound this trait by that so that they are coupled?
There was a problem hiding this comment.
I'm not sure it adds much, and neither trait is used by the other directly so they aren't strictly tied. If you think it makes sense I'll defer to your judgment.
| @@ -177,6 +236,89 @@ where | |||
| } | |||
| } | |||
|
|
|||
| const CHECKPOINT_FILENAME: &'static str = "checkpoint.txt"; | |||
There was a problem hiding this comment.
would it make sense to name this something related to journald?
There was a problem hiding this comment.
Sure. Any suggestions? journald-checkpoint.txt?
| } | ||
| let readonly = std::fs::metadata(&data_dir) | ||
| .map(|meta| meta.permissions().readonly()) | ||
| .unwrap_or(true); |
There was a problem hiding this comment.
this doesn't make much sense if the metadata call fails we can still readonly, wouldn't this indicate that maybe the file/dir doesn't exist?
There was a problem hiding this comment.
That was moved from the file source as-is. The check for the directory existing happens immediately above this, so this can only fail if stat fails on an existing directory, which is almost certainly also a permissions error (or kernel bug). I can add an additional error case if you think it makes sense.
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
Signed-off-by: Bruce Guenter <bruce@untroubled.org>
|
@LucioFranco do you still approve this or would you like to review it again since I pushed some additional changes? |
This implements checkpointing support for the journald source, which allows us to restart importing journald logs from after the most recently handled entry.
Ref #327