Allow configuration of the GC log file via an environment variable

[ankon]

Enabling GC logging works now by setting the environment variable ES_GC_LOG_FILE
to the full path to the GC log file. Missing directories will be created as needed.

The ES_USE_GC_LOGGING environment variable is no longer used.

Closes #8471
Closes #8479

Note that the default name of the file was already proper for Windows, so the fix for #8471 effectively is "do what windows did".

[clintongormley]

Hi @ankon

Thanks for this PR. I'm not much of a shell scripter, but I'm wondering if those assignments need to be wrapped in quotes, in case a path contains spaces?

[clintongormley]

In fact, a number of (existing) things fail if a path contains spaces: #8615

[spinscale]

Hey @ankon

this PR makes a lot of sense and we would like to get it in, but changes the default path of the gc.log to something else than /var/log/ means that it will not work for our packages? Can you maybe add that ES_GC_LOG_FILE variable to src/rpm/sysconfig/elasticsearch and src/deb/default/elasticsearch as well, so we can fall back to /var/log/elasticsearch/gc.log as path in our packages?

would you be willing to update this PR (as well as quoting the paths in the batch script)? We are happy to help though!

[ankon]

Missed the notifications, will have a look for the two raised issues.

[ankon]

@spinscale can you have a look over 669d99f? I hope I found all places where this should be referenced, but I don't use debian and didn't figure out how to build the RPM to test. Also, it seems ES_USE_GC_LOGGING is actually off in the packages, which maybe is something to be changed?

@clintongormley I saw similar issues to #8615, b657693 and 9611e72 fix those for me. I tested this by building master, and then running _JAVA_LAUNCHER_DEBUG=true ES_USE_GC_LOGGING=true sh -x bin/elasticsearch to see that all arguments are properly parsed. I did leave the fixes on this branch though, but could pull them into a separate PR if you think that makes more sense.

[spinscale]

@ankon if you do not set USE_GC_LOGGING in the init scripts (in case the ES_GC_LOG_FILE is set), then logging will not be activated, right? So this should also be a parameter in the defaults file I think

[spinscale]

also, thinking about the current need for two env vars, would it potentially make more sense to replace ES_USE_GC_LOGGING with ES_GC_LOG_FILE completely? If it is set, we log somewhere, without the need to set an extra environment variable for enabling/disabling. What do you think?

Yes, this would break bwc, but I think it simplifies things.

[ankon]

@spinscale, yes, you will need to explicitly enable GC logging before setting the name of the log file will do anything.

For the package case I think this should be the default (you do have a reasonable place to put the log, and there is no reason for not having the logs: worst situation ever to run out of memory and then figure out you don't have logs to show, and devs might not even be able to help you enable it because they don't know the package you're using).

For cases where packages aren't used the same argument holds for having logs enabled by default, but in those cases you have the choice where to have the log: default in the logs/ directory, if variable is set then where that one points to.

IOW: Yes, I'd be very much in favor of dropping the ES_USE_GC_LOGGING by enabling GC logging by default. :)

[spinscale]

Hey,

I took your PR as a base and fixed some things (and still need to test the packages), check it out here spinscale@671fe15

• Removed the USE_GC_LOGGING environment variable completely
• Made sure that the RPM does not do GC logging by default
• Added docs about the existence of the ES_GC_LOG_FILE environment variable

If you are good with this and we finished testing, we could get your PR in, if you add my changes. What do you think?

[ankon]

If I understand the packaging scripts correctly then for debian we now have GC logging enabled by default (to /var/log/elasticsearch/gc.log), and it can be disabled by setting the default explicitly to an empty value.

For the rpms it looks reverse: GC logging is disabled by default, and can be enabled by uncommenting the line in the sysconfig file.

I think it would make sense to have the packages behave in the same way, which according to the doc changes in spinscale/elasticsearch@671fe15 would be that src/deb/init.d/elasticsearch should have it commented?

Re combining/adding: sure, what's the procedure for that? Merge your commit(s) into my branch?

[spinscale]

good catch, fixed that in spinscale@ef8bd13

I'd be happy if you merge my commit into your branch, as I dont want to take your credit away here by just pushing my commit, as it is merely based on your work with minor modifications. Does that work for you?

[ankon]

Sounds good. I'm still learning my ways through git, but I just cherry-picked that commit now, and it looks good in the diff :)

[ankon]

Anything on my side to be done to get this merged?

[spinscale]

Hey,

looks good. Can you rebase & squash and then its ready from my point of view.
Sorry, this one slipped under my rader for too long.

[ankon]

Hi, I squashed them together on top of the (then-current :D) master.

While doing that I fixed a minor issue for Windows: it didn't create the directories. This was tested with a Windows 10 cmd.exe, and worked as expected. Note that while the bat code handles them, the JVM does not actually allow paths with spaces for the GC log.

[spinscale]

just merged it in, thx a lot for helping and your patience!