[ISSUE-115] Add Logstash Centralised Pipeline Management #151
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
RobsonSutton
force-pushed
the
issue-115
branch
from
b992598 to
acc9046
Compare
|
@tobio -Please can you take a look at this one when you have a spare sec? Cheers! 👍 Note: First full feature PR so please do shout if there is a better approach i should take with anything defined or if any parts don't make sense! |
|
jenkins test this |
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
Nice addition! Couple of comments in the code.
| Default: nil, | ||
| }, | ||
| }, | ||
| "pipeline_settings": { |
There was a problem hiding this comment.
IMO it would be nicer to have explicit schema items for the handful of pipeline.* available in Logstash. We'd be able to validate individual settings (both keys and values) within the provider rather than simply allow a free form map.
It would also be consistent with how we're now treating index settings.
WDYT?
There was a problem hiding this comment.
I actually started attempting to do this before throwing my toys out of the pram and going with the simpler approach, will take another look at this today though to see how to achieve this!
There was a problem hiding this comment.
Bit more testing and maybe a few tweaks left, nearly got it working, just some weird behaviour surrounding defaults when not defining the pipeline_settings block in the resource. Will take another look tomorrow 👍
There was a problem hiding this comment.
@tobio - My brain clearly wasn't working the other day as I was building based on the deprecated approach in index settings... apologies for this!
I have just updated the approach to follow the newer one in 1a45bda, this has also solved the issue I was having with the defaults not being applied when pipeline_settings {} was not set, so win win!
There was a problem hiding this comment.
Exactly what I was looking to test, I'd missed the update here and noticed the code I pulled down looked different to what I remember :). Thanks for this!
|
jenkins test this |
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
Looking good, one small comment on the code but really I just want to test some specifics out here before 🍏
Yeah if you are able to test then that would be massively appreciated! 👍 I have been trying to figure out how to define either an empty default or pass through the defaults defined for each setting when the |
|
jenkins test this |
| Description: "The number of parallel workers used to run the filter and output stages of the pipeline.", | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 1, |
There was a problem hiding this comment.
Logstash will default to the number of cores on the host. We should drop the default here and only specify this if it's set to something other than 0.
There was a problem hiding this comment.
Updated to validate >=1 for input, is this what you meant? can't think of a case where a user would want workers set to 0 (unless I'm missing something) and as you said this default will be managed on the elasticsearch side 👍
| }, | ||
| }, | ||
| // Pipeline Settings | ||
| "pipeline_batch_delay": { |
There was a problem hiding this comment.
There's a bunch more settings we should look to support. It looks like:
pipeline.unsafe_shutdown
pipeline.plugin_classloaders
pipeline.ordered
pipeline.ecs_compatibility
queue.max_events
queue.page_capacity
queue.checkout.acks
queue.checkout.retry
queue.drain
I'm happy to add these in a follow up if you're short of time, it would be good to have a more complete group of options though.
There was a problem hiding this comment.
I was just focusing on the ones visible via the UI initially, I've just updated to include those mentioned too though 👍 Have removed defaults for all to reduce plan noise since all have defaults being set on the API side, happy to re-add these defaults if you think they are required at all though?
| settings := make(map[string]interface{}) | ||
| for key := range settingsKeys { | ||
| tfFieldKey := convertSettingsKeyToTFFieldKey(key) | ||
| settings[key] = d.Get(tfFieldKey) |
There was a problem hiding this comment.
With the pipeline.workers comment above this should likely be something like
| settings[key] = d.Get(tfFieldKey) | |
| if setting, ok := d.GetOk(tfFieldKey); ok { | |
| settings[key] = setting | |
| } |
I wonder if we should instead extract this method into utils and re-use the index implementation instead.
There was a problem hiding this comment.
Yeah I have no clue why I copied and simplified this, have extracted to utils now though as suggested 👍
|
jenkins test this |
|
jenkins test this |
|
jenkins test this |
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
LGTM. Thanks for this! Really appreciate the patience through the review :)
No probs at all! 👍 Thanks for helping throughout the process, am new to Go and provider dev so has been good getting some feedback on better approaches, usage of syntax etc! |
3 participants
Uh oh!
There was an error while loading. Please reload this page.