search

voxpupuli / puppet-logstash

Puppet module to manage Logstash
https://forge.puppet.com/elastic/logstash
Apache License 2.0
191 stars 299 forks source link

remove deprecated GC JVM options, fixes #397 #415

Closed xorpaul closed 1 year ago

xorpaul commented 2 years ago

Removes the need for the workaround by overwriting deprecated JVM options in https://github.com/voxpupuli/puppet-logstash/issues/397

smortex commented 2 years ago

lol, this is logstash, not elasticsearch. I guess the same apply: can we sync with https://github.com/elastic/logstash/blob/main/config/jvm.options ?

xorpaul commented 2 years ago

The default jvm options from https://github.com/elastic/logstash/blob/main/config/jvm.options still contain Concurrent Mark Sweep (CMS) GC algorithm parameters, which do not work with more recent java versions.

That's why the official documentation even states that you need to remove those options if you plan to use the supported JDK 17 version: https://www.elastic.co/guide/en/logstash/current/getting-started-with-logstash.html#jdk17-upgrade

To be honest if someone wants to make logstash run with JDK 11 and then really need the CMS GC algorithm parameters (I have not tested that) they should add those options explicitly to logstash::jvm_options not that all others need to figure out how to disable these default options, which renders their logstash service unstartable.

The best option would be if we could check the used java version and set the JVM options defaults accordingly.

juliantaylor commented 2 years ago

you could support older jdk's by using the 11-13: syntax the components support, it means the options are only added when running in java version 11, 12 or 13. similar 11- means 11 or newer.

Not sure which version of logstash supports that but elasticsearch 6.4 does which is pretty old.

But regardless how it is solved it would be nice to have this fixed and released soon, the module is getting more and more unusable as java 11 disappears.

smortex commented 2 years ago

That's why the official documentation even states that you need to remove those options

Oh god… What an Enterprise™ :hankey: Product :facepalm:

@juliantaylor suggestion seems quite good. @xorpaul do you mind doing elastic's job and provide working default for all versions of Java so that we do not break backward compatibility?

juliantaylor commented 2 years ago

I'll provide a PR with the updated options and make the module defaults override-able too to avoid needing to change the module in the future for this.

juliantaylor commented 2 years ago

see https://github.com/voxpupuli/puppet-logstash/pull/417

xorpaul commented 2 years ago

I'm also OK with closing this in favor of #417

kenyon commented 1 year ago

Done in #417.