[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1232208598 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1232061276 ## flink-dist/src/main/resources/flink-conf.yaml: ## @@ -16,6 +16,7 @@ # limitations under the License. +env.java.opts.all: --add-exports=java.base/sun.net.util=ALL-UNNAMED --add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.text=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.locks=ALL-UNNAMED Review Comment: > add a generic comment stating that these APIs are needed to be exposed for the Flink to run under Java 17+ That we can (and should) definitely do. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231982022 ## .mvn/jvm.config: ## @@ -1 +1,7 @@ -XX:+IgnoreUnrecognizedVMOptions +--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED Review Comment: as mentioned elsewhere we can't add a comment in this file. I will double-check whether this one specifically is still required. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231984611 ## flink-dist/src/main/resources/flink-conf.yaml: ## @@ -16,6 +16,7 @@ # limitations under the License. +env.java.opts.all: --add-exports=java.base/sun.net.util=ALL-UNNAMED --add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.text=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.locks=ALL-UNNAMED Review Comment: At the end of the day though there's currently no problem with having too many of these. I'm inclined to go with the current set and maybe trim it down later. It would require quite a bit of time to exclude this one by one to figure out what exactly requires which one, and I don't really want to block the PR on that :/ ( I could run some things over the weekend though?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r123126 ## .mvn/jvm.config: ## @@ -1 +1,7 @@ -XX:+IgnoreUnrecognizedVMOptions Review Comment: We cant add a license header to this file. I believe the rat plugin is aware of that, because it hasnt complained so far. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231130267 ## flink-dist/src/main/resources/flink-conf.yaml: ## @@ -16,6 +16,7 @@ # limitations under the License. +env.java.opts.all: --add-exports=java.base/sun.net.util=ALL-UNNAMED --add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.text=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.locks=ALL-UNNAMED Review Comment: But to an extend it is the combination of module configs and what the yarn/e2e tests revealed :see_no_evil: I know it is a bit of a problem maintenance-wise. At one point I tried setting in the root pom (similar to `surefire.module.config`) and injecting it into the config during the build. But there the new-lines from the pom.xml were preserved and the result was incorrect... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231126400 ## .mvn/jvm.config: ## @@ -1 +1,7 @@ -XX:+IgnoreUnrecognizedVMOptions +--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED Review Comment: krb5 is for some reason required for the yarn kerberos tests. The other stuff is for various plugins. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231125800 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231122366 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231118408 ## flink-python/pyflink/pyflink_gateway_server.py: ## @@ -254,8 +254,10 @@ def launch_gateway_server_process(env, args): [construct_flink_classpath(env), construct_hadoop_classpath(env)]) if "FLINK_TESTING" in env: classpath = os.pathsep.join([classpath, construct_test_classpath()]) -command = [java_executable, jvm_args, "-XX:+IgnoreUnrecognizedVMOptions"] + jvm_opts \ -+ log_settings + ["-cp", classpath, program_args.main_class] + program_args.other_args +command = [java_executable, jvm_args, "-XX:+IgnoreUnrecognizedVMOptions", + "--add-opens=jdk.proxy2/jdk.proxy2=ALL-UNNAMED"] \ Review Comment: yes. Basically python uses java proxies to communicate, and also does reflection work against these proxies afaict. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231117453 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.locks=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231114423 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231113165 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231112231 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231110020 ## flink-core/pom.xml: ## @@ -33,6 +33,25 @@ under the License. jar + +--add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231107375 ## .mvn/jvm.config: ## @@ -1 +1,7 @@ -XX:+IgnoreUnrecognizedVMOptions +--add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED Review Comment: I don't _think_ so; I havent tried it but according to Maven all these lines are passed as-is to the JVM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231106320 ## flink-connectors/flink-connector-files/pom.xml: ## @@ -34,6 +34,13 @@ under the License. jar + +
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231105301 ## flink-dist/src/main/resources/flink-conf.yaml: ## @@ -16,6 +16,7 @@ # limitations under the License. +env.java.opts.all: --add-exports=java.base/sun.net.util=ALL-UNNAMED --add-exports=java.rmi/sun.rmi.registry=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-exports=java.security.jgss/sun.security.krb5=ALL-UNNAMED --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.text=ALL-UNNAMED --add-opens=java.base/java.time=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.locks=ALL-UNNAMED Review Comment: eh that might be difficult in hindsight :sweat_smile: Ill see what I can do -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794:
URL: https://github.com/apache/flink/pull/22794#discussion_r1231023922
##
pom.xml:
##
@@ -196,6 +196,12 @@ under the License.
*/
+
Review Comment:
> [-]{2}add
This looks as weird as it does because you can't have double-dashes in xml
comments :facepalm:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #22794: [FLINK-25002][build] Add java 17 add-opens/add-exports JVM arguments
zentol commented on code in PR #22794: URL: https://github.com/apache/flink/pull/22794#discussion_r1231022699 ## flink-end-to-end-tests/flink-queryable-state-test/pom.xml: ## @@ -107,6 +107,9 @@ org.apache.flink.streaming.tests.queryablestate.QsStateClient + + java.base/java.util + Review Comment: This is a handy alternative in case you run a self-contained jar; you can encode the opens/exports in the jars manifest. Unfortunately this only works if the you use the `-jar` command; we can't use it for the distribution :cry: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
