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

Update go-txfile and dependencies #7859

Merged
merged 12 commits into from
Aug 8, 2018
Merged

Conversation

urso
Copy link

@urso urso commented Aug 3, 2018

Resolves #7849, #7867, #7720

This PR updates go-txfile to v0.0.3 and golang.org/x/sys packages (go-txfile requires more recent version).

The update fixes a number of Windows related bugs in spooling to disk and replaces a call to fsync with fcntl+F_FULLFSYNC on darwin.

The error messages generated by go-txfile will be more detailed. We will further improve error handling and error reporting in the spool in follow up PRs.

The changes to Changelog.asciidoc for all user visible fixes and improvements.

@andrewkroh
Copy link
Member

Can TestProduceConsumer be un-skipped with this change? That would resolve #7720.

@urso
Copy link
Author

urso commented Aug 7, 2018

@andrewkroh yes, it should be unskipped. Thank you!

@urso urso force-pushed the upd/go-txfile-0.0.2 branch from 94fc470 to 4b4f2a1 Compare August 7, 2018 11:29
@urso urso changed the title [WIP] update go-txfile and dependencies Update go-txfile and dependencies Aug 7, 2018
@urso urso added the review label Aug 7, 2018
@urso urso requested a review from ph August 7, 2018 18:12
@urso urso force-pushed the upd/go-txfile-0.0.2 branch from 8e0efee to d5402d2 Compare August 7, 2018 18:18
@urso urso added the needs_backport PR is waiting to be backported to other branches. label Aug 7, 2018
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Minor comments concerning errors that could be hidden and might need to be exposed in the logger.

for len(b.pending) > 0 {
n, err := b.queue.Writer().Write(b.pending)
n, err := w.Write(b.pending)
Copy link
Contributor

Choose a reason for hiding this comment

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

This error will get hidden, should we log it as debug?

Copy link
Author

Choose a reason for hiding this comment

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

It's not logged on purpose. There can be a many reasons why this fails. E.g. IO error vs. spool is full. The later one is no error, but expected behavior. In future PRs we will check for the causes + handle/log them appropriately.

avail, err := reader.Available()
if err != nil {
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we log it?

Copy link
Author

Choose a reason for hiding this comment

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

This error will be logged by the beat. The spool constructor fails if this call fails.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM

@ph ph merged commit 10f41be into elastic:master Aug 8, 2018
@urso urso added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 8, 2018
urso pushed a commit to urso/beats that referenced this pull request Aug 8, 2018
* Update go-txfile

* Update golang.org/x/sys

go-txfile requires a newer version of golang.org/x/sys

* Update spool to take API changes into account

* Ensure spool opening root cause is included when printing the error message

* cleanup vendor after govendor fails

* Update notice file

* Update changelog

* Fix build

* Update go-txfile to 0.0.3

(cherry picked from commit 10f41be)
@urso urso added the v6.4.0 label Aug 8, 2018
urso pushed a commit to urso/beats that referenced this pull request Aug 8, 2018
* Update go-txfile

* Update golang.org/x/sys

go-txfile requires a newer version of golang.org/x/sys

* Update spool to take API changes into account

* Ensure spool opening root cause is included when printing the error message

* cleanup vendor after govendor fails

* Update notice file

* Update changelog

* Fix build

* Update go-txfile to 0.0.3

(cherry picked from commit 10f41be)
kvch pushed a commit that referenced this pull request Aug 8, 2018
* Update go-txfile

* Update golang.org/x/sys

go-txfile requires a newer version of golang.org/x/sys

* Update spool to take API changes into account

* Ensure spool opening root cause is included when printing the error message

* cleanup vendor after govendor fails

* Update notice file

* Update changelog

* Fix build

* Update go-txfile to 0.0.3

(cherry picked from commit 10f41be)
kvch pushed a commit that referenced this pull request Aug 8, 2018
* Update go-txfile

* Update golang.org/x/sys

go-txfile requires a newer version of golang.org/x/sys

* Update spool to take API changes into account

* Ensure spool opening root cause is included when printing the error message

* cleanup vendor after govendor fails

* Update notice file

* Update changelog

* Fix build

* Update go-txfile to 0.0.3

(cherry picked from commit 10f41be)
@urso urso added the libbeat label Aug 14, 2018
@urso urso deleted the upd/go-txfile-0.0.2 branch February 19, 2019 18:29
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* Update go-txfile

* Update golang.org/x/sys

go-txfile requires a newer version of golang.org/x/sys

* Update spool to take API changes into account

* Ensure spool opening root cause is included when printing the error message

* cleanup vendor after govendor fails

* Update notice file

* Update changelog

* Fix build

* Update go-txfile to 0.0.3

(cherry picked from commit b6e44a6)
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.

[libbeat] - Spool to disk permissions issues on Windows
3 participants