Merged
Conversation
This was referenced Aug 31, 2017
Closed
Contributor
|
I will test this shortly. |
Contributor
|
jordansissel
approved these changes
Aug 31, 2017
Contributor
jordansissel
left a comment
There was a problem hiding this comment.
I tested on my home workstation and tests pass.
Code LGTM.
| "total_in_millis"=>Numeric, | ||
| "percent"=>Numeric, | ||
| "load_average" => { "1m" => Numeric } | ||
| # load_average is not supported on Windows, set it below |
Contributor
There was a problem hiding this comment.
I confirmed manually that on Elasticsearch (I used ES 5.5.0) only has load_average on Linux. On Windows, the cpu object only has percent.
Contributor
|
@colinsurprenant Thanks for solving these issues! |
7e46d1b to
8b89fec
Compare
use rm_rf to delete dir and shutdown pipeline after run avoid the use of rescue nil, bad practice do not mock close as it prevents closing the file an prevents removing it on Windows cleanup temporary files and relax file name check for Windows fix paths handling for Windows flag the read file string as UTF-8 which is what we expect fix Windows issues ignore load_average on windows windows safe URI cleanup and fix file handling for windows wait for pipeline shutdown to complete revert to original puts cleanups use environment for windows platform check fix hash path wait for pipeline thread to complete after shutdown
8b89fec to
21dbde2
Compare
Contributor
Author
|
I decided to rebase into 2 separate commits, one for the specific tests specs fixes and the other for the pipeline stop action fix which for some reason only surfaced in windows tests but is definitely a potential problem everywhere. |
Contributor
Author
|
merged in master, 6.x and 6.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modifications to have a green build on Windows.
Tested for successful
rake test:corelocally on Windows Server 2012 and on OSX.These are mostly spec fixes.
The only non-spec significant change which should prevent potential problems on all platforms is in
logstash-core/lib/logstash/pipeline_action/stop.rbwhere after issuing a pipeline shutdown, we were not waiting on the pipeline thread to complete.