-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add OpenJDK 11 action loop runtime #82
base: master
Are you sure you want to change the base?
Conversation
For canonical discussion on illegal access: See blog discussion of migrating to Java 11, addressing the issue as well: |
NOTE: |
README.md
Outdated
``` | ||
|
||
The `$user_prefix` is usually your dockerhub user id. | ||
|
||
Deploy OpenWhisk using ansible environment that contains the kind `java:8` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Distinguish and/or create examples for java8 and java11
075821e
to
9578524
Compare
The conflict has been resolved |
@upgle First, I wanted to apologize for not seeing your resolving conflicts on Set 24th. I have been busy and traveling and am now back and have time to dedicate to openwhisk and the java runtime. I hate to ask this, but could you resolve the gradle config conflicts and I promise to review/merge as soon as you complete. Feel free to message me directly in Slack if you like to assure I know the minute you are done or just to chat and ask questions. Many thanks! |
@mrutkows I'm sorry for the late response too, I'll resolve all conflicts soon. When I made this Java11 runtime, I didn't consider the actionLoop.
If we don't need to care lagacy code anymore, I'll convert this runtime as actionLoop based runtime as well. |
+1 for one version - 2 versions means twice the maintenance. |
Ok, I'll convert this code for action-loop based. And I have a question @rabbah. Currently, the python runtime has both actionLoop-based and non-actionLoop-based runtimes. Do you plan to support both types even if the python version is updated? I just wonder if actionLoop has become the standard specification for the runtime. https://github.com/apache/openwhisk-runtime-python/tree/master/core |
c8c59db
to
8654b2b
Compare
jars.append(target_dir) | ||
cmd = """#!/bin/bash | ||
cd "%s" | ||
/opt/java/openjdk/bin/java --add-opens java.base/java.util=ALL-UNNAMED --illegal-access=permit -Dfile.encoding=UTF-8 -cp "%s" Launcher "%s" "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
From java 9 version, it outputs below warning messages when accessing encapsulated field by reflection.
WARNING: An illegal reflective access operation has occurred
WARNING: Please consider reporting this to the maintainers of Main
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
Source code using reflection
- core/java11actionloop/lib/src/Launcher.java
if ("java.util.Collections$UnmodifiableMap".equals(cl.getName())) {
Field field = cl.getDeclaredField("m");
field.setAccessible(true); // <- here
So, I added --add-opens
and --illegal-access
VM options to access java.util.Collections$UnmodifiableMap
.
--add-opens java.base/java.util=ALL-UNNAMED
--illegal-access=permit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed that is an horrible and ugly hack that unfortunately is the only way to set environment variables with jdk8. I was kinda of "forced" to add that in actionloop because I tried to make the actionloop 100% compatible but I really would like to change the API, and not use environment variables but system properties to pass environment variables.
I've converted the legacy java11 runtime to use action loop. |
@mrutkows Do you have any other comments on this? |
If the action loop proxy sets all the environment variables for the activation, then I posit the unsafe env setting in the jvm isn’t needed anymore and so the unsafe hack could be removed. I’d favor making sure the env vars are all set in the proxy before the jvm starts vs switching to properties as that’s a breaking change and will break action migration from java8 to java10 kinds. |
The actionloop cannot set the environment variables for the activation, it can set the environment variables BEFORE the process start but it cannot change environment variables of a launched process. No program can do this, there is in Unix an exec call where you can pass an environment variables array. It is the task of a the launched program to set environment variables. Indeed all the "action loops" does exactly this: read the action and sets the environment variables. This is easy in every language except java, where it has been decided you cannot do this. And the only way is the ugly hack of marking an in-memory variabile read-only as writable and write it. I proposed to remove this hack and instead accept that the activation values are passed as system properties. This requires to change and document the difference. Or use the unsafe code... |
My opinion is that we should NOT use --add-opens and --illegal-access, but instead convert the way values are passed as system properties, and document the change. I discussed this in mailing list. |
I agree that it's a bad idea to use flags like --add-opens and -illegal-access. I think the best solution would be to change the contract between the runtime and the user action code so that those hacks aren't needed. I think the "OpenWhisk variables" as I like to call them (the env vars that start with I made a Java 11 runtime to test this out: https://github.com/mattwelke/apache-openwhisk-runtime-java It works pretty well, as you can see from the screenshot in that repo's README: Note that I'm also using a technique I mentioned in apache/openwhisk-runtime-dotnet#51, where I suggest using the language's native map data structure instead of JSON for the user action contract (as the Node.js and Go runtimes already do) so that the user doesn't have to link to a JSON library dependency to build their action code. In Java, that means My custom runtime repo's example action shows it all put together, which is what's running in that IBM Cloud Function: import java.util.HashMap;
import java.util.Map;
public class Function {
public static Map<String, Object> main(Map<String, Object> params, Map<String, Object> owVars) {
System.out.println("Runtime version: " + Runtime.version());
printMap(params, "Params");
printMap(owVars, "OpenWhisk variables");
var output = new HashMap<String, Object>();
output.put("hello", "world");
output.put("answer", 42);
return output;
}
private static void printMap(Map<String, Object> map, String mapName) {
System.out.println(mapName + " entries:");
for (var entry : map.entrySet()) {
System.out.println("Key: " + entry.getKey() + " | Value: " + entry.getValue());
}
}
} Note that if that repo's layout looks weird to you, it's not you. :P I'm not using Maven or Gradle because I haven't gotten a chance to know them deeply yet. I'm learning Java because my work is pivoting to using it as our main language. I thought a fun way to learn Java would be to make Java 11 and Java 16 runtimes for OpenWhisk. But it's easier for me to put together a custom build system using Bash and checked in JAR files than to figure out Maven or Gradle right now. I understand that if we add a Java 11 runtime to the project, we'd want to make its structure fit our existing Gradle build system. |
The second set of parameters is called context in AWS Lambda lingo. |
This is actually why I leaned towards a context parameter instead of system properties for Java. The context parameter is a pattern that will work all runtimes. It would let OpenWhisk continue to have that consistency. System properties are a Java only concept, so this would only work for Java and if another runtime ever has this come up too, then we'd have to use a language-specific fix for that runtime too. Alternatively, we could say that consistency is only a goal within a particular language's runtimes. Maybe as long as all Java runtimes use system properties, we're good. And then it's okay to use other techniques (like JsonObject for C#, and language native dictionaries for Node.js and Go). It depends on what kind of consistency design goals we have. Of these two types of consistency, my gut feeling is that consistency across languages is better, using language native dictionaries and the context parameter. When an OpenWhisk user makes the jump from one language to another, like me when I moved from Node.js to C# when I wanted a compiler, and then C# to Java when my work began using Java and I wanted to skill up on it, then the user has less context shifting to worry about. |
Are you prepared to push this change forward for all the runtimes ;) |
While I wouldn't say I'm skilled enough in all of these languages to have made the runtimes from scratch, I think this is actually a simple change to make. I found it easy to make in Java. I'd say that my strongest languages are Node.js and Go right now, and that I'd feel comfortable pushing this change forward in the following runtimes, all of which I've coded at least a bit throughout my life as a programmer:
The other runtimes, I haven't ever ran a single line of code in them (like Swift) so there'd probably be some friction there, getting set up and maybe running into little things that would be obvious to someone with experience with the languages. I'd appreciate some help from others to step in and do those ones. As we discussed on Slack, there was a reason it was done with environment variables in the first place, to enable composition. And we'd want to explore whether composition would be hurt by changing it this way first. |
Thanks @mattwelke for leading this discussion. There will def be help once the proposal matures. |
This PR is to support OpenJDK 11. #78