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.

Sign up for GitHub

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

Jump to bottom

logstash testing #927

Merged
merged 23 commits into from
Feb 9, 2016
Merged

logstash testing #927

merged 23 commits into from
Feb 9, 2016

Conversation

urso
Copy link

@urso urso commented Feb 4, 2016

add more tests + potential fixes to logstash output mode

@urso urso added in progress Pull request is currently in progress. libbeat labels Feb 4, 2016
@urso urso mentioned this pull request Feb 5, 2016
Closed
@urso
Copy link
Author

urso commented Feb 6, 2016

Sorry for big PR. changes in this PR are mostly focused on adding unit tests to output modes and logstash client. All other changes are due to fixes and and refactorings to create unit tests.

  • refactor (splitting up) internal functionality into specialised types for adding unit tests:
    • backoff behavior
    • window sizing
    • logstash transport mode (TCP/TLS)
    • logstash protocol vs. client with connection
    • have more logstash client tests running on actual TCP connections instead of fake connection objects (helps with reproducibility)
  • fixes
    • logstash client entering infinite loop on too many timeout errors
    • do not retry events that are not json-encodable
    • large timeouts on backoff blocking output plugin shutdown (only for load balancer, single mode still suffers problem)
    • set default window size (bulk_max_size) to 2048 in logstash client (for some reason it was set back to 1024 in code)
    • raise an error if invalid sequence number in ACK message is received (bigger than max expected)
  • other improvements:
    • more detailed debug output in logstash client
    • client unit tests based on test-driver, so unit tests can be re-used with alternative client implementations (e.g. pipelining, new protocol)
    • re-use backoff implementation for balanced and single output mode + add backoff to fail-over mode
    • simplify connection mode initialization based on config
    • simplify load-balancer worker loop
    • load balancer worker forces reconnect/ping if error in send-loop

@urso urso added review and removed in progress Pull request is currently in progress. labels Feb 6, 2016
@urso
Copy link
Author

urso commented Feb 6, 2016

for comparison:

This was referenced Feb 7, 2016
Merged
Closed
Merged
@urso urso added in progress Pull request is currently in progress. review and removed review in progress Pull request is currently in progress. labels Feb 8, 2016
andrewkroh
andrewkroh reviewed Feb 8, 2016
View reviewed changes
libbeat/outputs/logstash/protocol_test.go
if err != nil {
return nil, err
}
// dataBuf.ReadFrom(streambuf.NewFixed(tmp))
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to remove this commented out code?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, can be removed. thanks

@andrewkroh
Copy link
Member

Looks good!

@urso urso force-pushed the enh/ls-testing branch 2 times, most recently from c1384f1 to 676d1bd Compare February 8, 2016 23:30
urso added 12 commits February 9, 2016 00:46
Error if invalid sequence number is received
4fd5a4d
move sync logstash client to sync.go
f02d14e
Add logstash client tests with failing connection on first message
7bb5f7a
Simplify balance mode worker loop
ec4aa54 
- simplify worker loop only reconnecting on failure
- re-use backoff state between multiple calls/messages
- Enforce reconnect if sendloop returned
single mode: reuse backoff state between multiple calls
3a607a2
Add simple unit test handling connection i/o timeout
64f55c4
Protect transport from panic if closed multiple times
2bf66da
Add multi-timeout errors unit test
95b756a
Fix infinite loop if too many timeout errors have been handled
3315bfc
On LS io timeout return timeout error to output mode
79a1bd7 
By returning the io error to the output mode, the mode can apply some backoff
between multiple failures + number of send attempts gets increased more
correctly. This way, even if timeout is 'transient', error handling strategy
implemented by output modes is still in place.
Simplify logstash client error handling
050978d 
simplify error handling in logstash client and always enforce a reconnect
on failure
Add changelog entries
d6936ee
andrewkroh added a commit that referenced this pull request Feb 9, 2016
@andrewkroh
Merge pull request #927 from urso/enh/ls-testing
67a30b8 
logstash testing
@andrewkroh andrewkroh merged commit 67a30b8 into elastic:master Feb 9, 2016
@urso urso deleted the enh/ls-testing branch February 11, 2016 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
libbeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@urso @andrewkroh