fabric8io-images / s2i

OpenShift S2I images for Java and Karaf applications
Apache License 2.0
70 stars 84 forks source link

Change gc log option for java11 #217

Closed muff1nman closed 5 years ago

muff1nman commented 5 years ago

Addresses #213. Not yet tested.

vorburger commented 5 years ago

Not yet tested

216 can test this - do you want to try running that against this?

muff1nman commented 5 years ago

Hmm, so the logic added here does not work unless the user sets JAVA_MAJOR_VERSION because as far as I can tell that environment variable isn't setup anywhere automatically. Is there a good reason that variable isn't set automatically? Naively, I imagine one could parse the output of java -version, but that just starts getting into the weeds a bit. However, if there is to remain one run-java-sh for all java versions, I don't see how to avoid doing so especially in cases like this where backwards compatibility is dropped.

muff1nman commented 5 years ago

Okay since I enjoy a good bush whacking, I wrote the following snippet that I think could work to grab the JAVA_MAJOR_VERSION. So far tested on Java 1.8 and Java 11:

java -XshowSettings:properties -version 2>&1 | grep -Po '(?<=\sjava.version = )(1\.)?\K[^.]*'

Again, I'm not sure I enjoy having to execute the java binary to get the version, but I can't think of a more robust way to do so. IMO it would make more sense to ship different versions of run-java.sh with the different docker images.

rhuss commented 5 years ago

@muff1nman your approach looks fine to me. Let me briefly explain the motivation behind https://github.com/rhuss/run-java-sh :

So it would be perfectly fine to support a JAVA_MAJOR_VERSION set from the outside of the script (e.g. when using in this image we can set this env var accordingly to this image), and possibly fallback to autodetection if not set.

wdyt, does this make sense ?

muff1nman commented 5 years ago

So you would propose that an ENV JAVA_MAJOR_VERSION=11 be added directly to the relevant docker files and the shared run-java-sh logic would try to autodetect if it finds that JAVA_MAJOR_VERSION has not been set?

vorburger commented 5 years ago

So you would propose that an ENV JAVA_MAJOR_VERSION=11 be added directly to the relevant docker files

yup, I think that's exactly what @rhuss is suggesting - and I would agree with him that just from a "performance of start-up time" point of view that would be much better than to have to run java -version each time the container starts.

and the shared run-java-sh logic would try to autodetect if it finds that JAVA_MAJOR_VERSION has not been set?

or may be you could even ditch that idea completely? It would seem OK to me if run-java.sh just aborted if the JAVA_MAJOR_VERSION environment variable is not set (but @rhuss has the last word on that).

muff1nman commented 5 years ago

Closing in favor of #219.

@vorburger @rhuss I like the idea of aborting if JAVA_MAJOR_VERSION isn't set to be defensive, but decided against it to be consistent with the other places it is used in run-java-sh.