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

Added java "wrapper" module #626

Closed
wants to merge 2 commits into from
Closed

Added java "wrapper" module #626

wants to merge 2 commits into from

Conversation

lgoldstein
Copy link
Contributor

As originally discussed in issue #623 I was looking for a way to start the JVM using a configuration file instead of hardwired / compiled values. I hope that the value of such a capability is obvious, but here are my main reasons for such a feature (I am sure you can find more):

  • Easily modify the configuration used to start the JVM – simply edit the configuration file and (re-) start the JVM. Obviously, a great advantage over the current situation where any changes in the startup parameters requires a new image (at least for my application which is automatically started at boot time). IMO, such changes, while not frequent, still occur – e.g., upgrading a JAR on the classpath, changing a –Dxxx=yyy definition, tuning the memory (-Xmx, -Xms, -Xss) parameters, etc…
  • Easily provide “profiles” via the @include feature. The user can have pre-configured “profiles” (e.g., development vs. production, distributed vs. standalone, etc.) and easily switch between them by simply editing the file and commenting in/out the “profile” – e.g.,
# The main configuration file – the un-commented line represents the boot profile.

@include development.conf
#@include production.conf
#@include standalone.conf
  • Avoid repetition of configuration by re-using “fragments” via the @include directive – e.g., the development vs. production configuration in the above example could be written as follows:
# development
@include common.conf

wrapper.java.additional.4=-Xmx968m
wrapper.java.additional.5=-Xms512m
wrapper.java.additional.6=-Dmy.app.mode=devel
# production
@include common.conf

wrapper.java.additional.4=-Xmx723m
wrapper.java.additional.5=-Xms256m
wrapper.java.additional.6=-Xss256k
# common
wrapper.java.additional.1=-Djava.awt.headless=true
wrapper.java.additional.2=-Djava.shell.repositories=http://mirrors.ibiblio.org/maven2
wrapper.java.additional.3=-Djava.shell.main.failure.stack.trace.delay=5000

The format is inspired by the Tanuki wrapper, although is not compatible with it (not a goal…).

About the implementation and source file location choice…

  • Initially, I followed @nyh’s advice and tried to locate it under apps. However, the more I thought about it and struggled to make it work, the more I became convinced that it belongs under the (core) modules. This choice proved fortunate as it greatly simplified the development and deployment as well as providing a smooth extension to the api.py module. However, even if we consider it conceptually it is not an application but more of a (core) module feature / capability.
  • The attached patches are deliberately split into 2 commits (the order is important and they should be applied accordingly)

** Move the original source code that is responsible for starting the JVM to a separate sub-folder - osv/java/jvm.

This choice makes sense as it creates the necessary logical separation between the JVM startup code (that has remained unchanged) and the (new) wrapper code. It also makes sure that the wrapper is compiled with the exact same settings as the java.so library (this proved to be a crucial issue). Note: nothing else has been changed in the sources or the location of the java.so artifact in the image.

BTW, in this context, even if you decide not to include the wrapper code, IMO this patch is worth applying (another reason why it is a separate commit), as it makes the source folders structure clearer – each functionality has its own folder (e.g., jni, balloon, etc.) – regardless of the language the source code is written.

** Create a new javawrapper module whose source resides under osv/java/wrapper

This includes adding a run_java_wrapper class/capability in the osv/scripts/module/api.py file, which means that users can simply do:

from osv.modules import api
api.require('javawrapper')

default = api.run_java_wrapper(configuration='/path/to/the/configuration/file')

Note: make sure you map correctly the configuration file to the declared location in the usr.manifest file

Which compared to the previous situation is an improvement (IMO) – see below:

from osv.modules import api
api.require('java')

default = api.run_java(classpath=['/usr/lib/jvm/java-shell/bootstrap-1.8.1.0.jar'],jvm_args=['-Xmx968m','-Xms512m','-Djava.awt.headless=true','-Djava.shell.repositories= http://repo1.maven.org/maven2,http://mirrors.ibiblio.org/maven2','-Djava.shell.main.failure.stack.trace.delay=5000'],args=['org.apache.commons.cli.shell.bootstrap.Bootstrap','-jsalogfine'])

All the “ugly” stuff is now in the configuration file and it can be easily edited…

@copumpkin
Copy link

Not sure if you noticed or if someone else told you, but this project appears to prefer patch contributions over the mailing list.

@lgoldstein
Copy link
Contributor Author

I know - I already sent a patch contribution consisting of 2 patches - one was merged, for some reason the other is delayed (for a long time) and I have not had any responses to my inquiry why is the 2nd patch delayed...

@geraldo-netto
Copy link
Contributor

Dear @wkozaczuk / @nyh ,

I think we could close this ticket once Waldek has added java8 alias recently
Maybe as a future improvement, define java as a generic alias for the most recent version of java
(java 9 with full profile?)

Kind Regards,
Geraldo Netto

@nyh
Copy link
Contributor

nyh commented Mar 11, 2018

I'm not sure the java8 alias is really relevant to this pull request. But changes that @wkozaczuk did fairly recently in splitting Java into modules/java-base and other components, probably made this pull request unusable as-is and I'm guessing it needs work if we want to apply it.

#include <map>

inline bool is_whitespace(char c) {
if ((c == ' ') || (c == '\t') || (c == '\r') || (c == '\n')) {

Choose a reason for hiding this comment

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

Why don't you return with this boolean expression right away?

@wkozaczuk
Copy link
Collaborator

Closing this pull request as the related issue is closed it would not apply anyway.

@wkozaczuk wkozaczuk closed this May 11, 2020
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.

6 participants