Skip to content
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

Read old records #1642

Merged
merged 2 commits into from Aug 16, 2017
Merged

Read old records #1642

merged 2 commits into from Aug 16, 2017

Conversation

lbudai
Copy link
Collaborator

@lbudai lbudai commented Aug 11, 2017

This PR supports read_old_records only for journal source.

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>
@lbudai lbudai force-pushed the read-old-records branch 2 times, most recently from 8ce86eb to 8803647 Compare August 11, 2017 14:07
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

1 similar comment
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@mochrul
Copy link
Contributor

mochrul commented Aug 11, 2017

(slight off topic)Hmm. I "received" similar complaint from the ubuntu, that sometimes the build is hung around the python tests (https://ubuntudiff.debian.net/q/package/syslog-ng). It may worth an issue.

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@lbudai
Copy link
Collaborator Author

lbudai commented Aug 12, 2017

Now, read-old-records implemented also for file-reader.
(this is not a port from pe, I've re-implemented this feature)

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Copy link
Collaborator

@furiel furiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Looks good. I have only one suggestion.
The new parameter: new_state(boolean) in (...)_apply_state might be confusing, at least in terms of variable naming.

options->read_old_records = TRUE;
Maybe a 3-state variable instead of this boolean would be better. Something like ENUM(TRUE,FALSE,OFF), when OFF would mean the original behaviour: when new_state is false. Thus we could maintain the original signature of log_proto_buffered_server_apply_state, and there is no need need of _apply_existing_state and _apply_new_state.

EDIT: Not relevant. It does not make sense to have 3state variable, because it only controls if we start at the beginning of a file or the end, and one of these should always be applied for new files.

@lbudai
Copy link
Collaborator Author

lbudai commented Aug 15, 2017

@furiel: thanks for the review note.
I wouldn't change the code because we would set read_old_records to OFF only when there is no existing persist state in the persist file: in this case we would need to override the read_old_records value in the options structure... and I think it is not as readable as the current version (it is possible to find a better name than new_state, but I don't have such a name at this time).

The key point here is that we applying an existing state or a new one: when existing one is applied then we continue to read logs from the position stored in the persist file, otherwise
the user can set the start position: head(read_old_records=yes) or tail(read_old_records=no).
When the user does not set this value, the default is yes.

@lbudai
Copy link
Collaborator Author

lbudai commented Aug 15, 2017

Hmm...the affile read-old-records will be handled in a separate PR as it needs some change in functionality.
But systemd-journal part of the PR can be merged.

@pzoleex
Copy link
Collaborator

pzoleex commented Aug 15, 2017

@kira-syslogng test this please test branch=pzolee-journal-source-read-old-record;

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@kira-syslogng
Copy link
Contributor

Build FAILURE, the tests were executed on test branch: pzolee-journal-source-read-old-record and test suite: functions

@pzoleex
Copy link
Collaborator

pzoleex commented Aug 15, 2017

@kira-syslogng test this please test branch=pzolee-journal-source-read-old-record;

@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: pzolee-journal-source-read-old-record and test suite: functions

@presidento presidento merged commit 91c8293 into syslog-ng:master Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants