Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change gc log option for java11 #217

Closed

Conversation

muff1nman
Copy link
Contributor

Addresses #213. Not yet tested.

@vorburger
Copy link
Collaborator

vorburger commented Jan 17, 2019

Not yet tested

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

Copy link
Collaborator

@vorburger vorburger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bigger small 😈 problem here.. the java/images/*/run-java.sh files are generated, so you cannot change them here in this project with a PR like this (if we did merge this, we would loose it again very soon when re-generating all those files via fish-pepper, as ./test.sh also does) ... as noted here, quote: we need a similar PR to this but instead against run-java-sh; then @rhuss will need to do a new release of run-java-sh and a bump the version to it here in this project (like in #214), and then a rebase #216 which should then pass. Hope this makes sense?

@muff1nman
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

rhuss commented Jan 21, 2019

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

  • It's supposed to be a general purpose startup script, which is especially useful when started from within a container.
  • It's not directly bound to a specific image
  • Its included/imported by the images like this here.

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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.

@muff1nman muff1nman closed this Feb 3, 2019
@vorburger vorburger mentioned this pull request Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants