You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2023/05/25 18:49:06 UTC
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19939
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Restricts jvm_automatic_add_opens to only apply to Java 9+ where the
option exists. Previously it would also include it in Java 8, which
caused the JVM to ignore all options in JAVA_TOOL_OPTIONS.
Tests for Java version by running $JAVA_HOME/bin/java -version and
parsing version from the first line. All JVM implementations are
expected to include the version in a quoted string, such as "1.8.0_42"
and "11.0.1".
Also added add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing: manually confirmed -agentlib options are present with both Java
8 and Java 11.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
2 files changed, 63 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/1
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7:
This mostly passed https://jenkins.impala.io/job/ubuntu-20.04-from-scratch-java11/.
There's a new Java 11 failure surfacing that I'm pretty sure is unrelated (build based on patch set 1 passed): the Text and BytesWritable signatures for BufferAlteringUdf treat empty string differently, and which signature matches a string is arbitrary (based on the order of Class.getMethods). I'm going to file that as a separate issue.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 18:13:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13162/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 16:54:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@311
PS2, Line 311: Unknown flags in JAVA_TOOL_OPTIONS causes Java to ignore everything,
We could create a test that sets something basic in JAVA_TOOL_OPTIONS and verifies that the JVM actually has that option after startup? (Or modify an existing test to fail if the jvm option doesn't get through.)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@315
PS2, Line 315: if (java_home != NULL) {
: cmd = (boost::filesystem::path(java_home) / "bin" / "java").string();
: }
For my curiousity: Do our docker images find this via JAVA_HOME or PATH with this change?
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@335
PS2, Line 335: if (std::stoi(matches.str(1)) < 9) return Status::OK();
Technically this can throw an exception if things go wrong. They shouldn't go wrong, but probably we should handle it anyway.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 May 2023 23:43:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc@313
PS4, Line 313: string cmd = "java";
> Java 8 is the most commonly used JVM version right now, and keeping its JVM
Other reviewers can chime in here. I can live with the unconditional approach if we think this java invocation is too clunky. But I'm getting more risk averse about touching anything for Java 8.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 18:31:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 12:34:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2: Code-Review+1
Thanks for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 May 2023 16:44:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19939
to look at the new patch set (#5).
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
If Java 8 detects unknown options in JAVA_TOOL_OPTIONS, it ignores the
whole string. Changes jvm_automatic_add_opens to update LIBHDFS_OPTS, as
it ignores individual unknown options when initializing the JVM.
Also adds add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing:
- manually confirmed -agentlib and LIBHDFS_OPTS from impala-config.sh
options are present with both Java 8 and Java 11.
- promoted test_jvm_mem_tracking to run in all strategies, as it's fast
and ensures JAVA_TOOL_OPTIONS is honored.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
M docker/daemon_entrypoint.sh
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 46 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/5
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9367/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 18:13:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13168/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 19:20:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19939
to look at the new patch set (#6).
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
If Java 8 detects unknown options in JAVA_TOOL_OPTIONS, it ignores the
whole string. Changes jvm_automatic_add_opens to update LIBHDFS_OPTS, as
it ignores individual unknown options when initializing the JVM.
Also adds add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing:
- manually confirmed -agentlib and LIBHDFS_OPTS from impala-config.sh
options are present with both Java 8 and Java 11.
- promoted test_jvm_mem_tracking to run in all strategies, as it's fast
and ensures JAVA_TOOL_OPTIONS is honored.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
M docker/daemon_entrypoint.sh
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 43 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/6
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@311
PS2, Line 311: Unknown flags in JAVA_TOOL_OPTIONS causes Java to ignore everything,
> We could create a test that sets something basic in JAVA_TOOL_OPTIONS and v
Turns out there is one, but it's an exhaustive test. Adding a fast test makes sense.
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@315
PS2, Line 315: if (java_home != NULL) {
: cmd = (boost::filesystem::path(java_home) / "bin" / "java").string();
: }
> For my curiousity: Do our docker images find this via JAVA_HOME or PATH wit
I added using PATH as a fallback because of the docker images.
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@335
PS2, Line 335: if (std::stoi(matches.str(1)) < 9) return Status::OK();
> Technically this can throw an exception if things go wrong. They shouldn't
As in try/catch? I think a DCHECK also would, and we control the regex. I can add it though.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 May 2023 17:28:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc@313
PS4, Line 313: string cmd = "java";
An alternative to this would be to take over creating the JVM ourselves using JNI_CreateJavaVM. Currently we rely on the HDFS C interface to do that for us. If we did it ourselves, we'd need to recreate some of the classpath logic and include LIBHDFS_OPTS.
However HDFS calls JNI_CreateJavaVM with vm_args.ignoreUnrecognized=1, and LIBHDFS_OPTS gets transformed into vm_args.options. Using LIBHDFS_OPTS rather than JAVA_TOOL_OPTIONS would allow us to supply unknown parameters, and skip checking the Java version.
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh
File bin/run-all-tests.sh:
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh@276
PS4, Line 276: export JAVA_TOOL_OPTIONS="$JAVA11_OPTIONS ${JAVA_TOOL_OPTIONS-}"
> Should we ignore these in java 8? It seems JAVA_TOOL_OPTIONS will be unusab
Yeah, that would make sense.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 16:18:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9367/
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 23:33:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Jun 2023 04:42:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has removed a vote on this change.
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13151/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 May 2023 21:17:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@313
PS2, Line 313: string cmd = "java";
This all feels a little fragile. It might be better to add a startup check that we can introspect a few modules, and rely on testing and documentation to cover the set of classes we need access to.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 May 2023 23:18:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@311
PS2, Line 311: Unknown flags in JAVA_TOOL_OPTIONS causes Java to ignore everything,
> Turns out there is one, but it's an exhaustive test. Adding a fast test mak
Done
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@313
PS2, Line 313: string cmd = "java";
> This all feels a little fragile. It might be better to add a startup check
Ack
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@315
PS2, Line 315: if (java_home != NULL) {
: cmd = (boost::filesystem::path(java_home) / "bin" / "java").string();
: }
> It's good to support falling back to PATH, but we can also export JAVA_HOME
Done
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@335
PS2, Line 335: if (std::stoi(matches.str(1)) < 9) return Status::OK();
> Right, with our regex there shouldn't be an issue. We could limit the numbe
Done
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 May 2023 20:55:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7: Code-Review+2
Carrying +1 from Joe and Zoltan. Thanks for fixing this!
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Jun 2023 00:55:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13133/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 May 2023 21:03:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13163/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 16:57:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Add-opens in LIBHDFS OPTS
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Add-opens in LIBHDFS_OPTS
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc@313
PS4, Line 313: string cmd = "java";
> An alternative to this would be to take over creating the JVM ourselves usi
Java 8 is the most commonly used JVM version right now, and keeping its JVM arguments unmodified is a nice property. I feel like this logic is doing what a calling script would have done if jvm_automatic_add_opens didn't exist. I think I prefer this approach to the unconditional approach.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 18:27:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/4/be/src/common/init.cc@313
PS4, Line 313: string cmd = "java";
> Other reviewers can chime in here. I can live with the unconditional approa
Reverted.
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh
File bin/run-all-tests.sh:
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh@276
PS4, Line 276: JAVA_OPTIONS+=" --add-opens=jdk.management.jfr/jdk.management.jfr=ALL-UNNAMED"
> Yeah, that would make sense.
Done
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 20:38:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19939
to look at the new patch set (#2).
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Restricts jvm_automatic_add_opens to only apply to Java 9+ where the
option exists. Previously it would also include it in Java 8, which
caused the JVM to ignore all options in JAVA_TOOL_OPTIONS.
Tests for Java version by running $JAVA_HOME/bin/java -version (or
"java" if JAVA_HOME is unset) and parsing version from the first line.
All JVM implementations are expected to include the version in a quoted
string, such as "1.8.0_42" and "11.0.1".
Also added add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing: manually confirmed -agentlib options are present with both Java
8 and Java 11.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
2 files changed, 61 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/2
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 4:
(1 comment)
Thanks for fixing this! Just have a minor comment.
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh
File bin/run-all-tests.sh:
http://gerrit.cloudera.org:8080/#/c/19939/4/bin/run-all-tests.sh@276
PS4, Line 276: export JAVA_TOOL_OPTIONS="$JAVA11_OPTIONS ${JAVA_TOOL_OPTIONS-}"
Should we ignore these in java 8? It seems JAVA_TOOL_OPTIONS will be unusable when running tests on java 8, though I never use it for run-all-tests.sh..
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Jun 2023 01:01:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19939
to look at the new patch set (#4).
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Restricts jvm_automatic_add_opens to only apply to Java 9+ where the
option exists. Previously it would also include it in Java 8, which
caused the JVM to ignore all options in JAVA_TOOL_OPTIONS.
Tests for Java version by running $JAVA_HOME/bin/java -version (or
"java" if JAVA_HOME is unset) and parsing version from the first line.
All JVM implementations are expected to include the version in a quoted
string, such as "1.8.0_42" and "11.0.1".
Also added add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing:
- manually confirmed -agentlib options are present with both Java
8 and Java 11.
- promoted test_jvm_mem_tracking to run in all strategies, as it's fast
and ensures JAVA_TOOL_OPTIONS is honored.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
M docker/daemon_entrypoint.sh
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 62 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/4
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19939
to look at the new patch set (#7).
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Restricts jvm_automatic_add_opens to only apply to Java 9+ where the
option exists. Previously it would also include it in Java 8, which
caused the JVM to ignore all options in JAVA_TOOL_OPTIONS.
Tests for Java version by running $JAVA_HOME/bin/java -version (or
"java" if JAVA_HOME is unset) and parsing version from the first line.
All JVM implementations are expected to include the version in a quoted
string, such as "1.8.0_42" and "11.0.1".
Also added add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing:
- manually confirmed -agentlib options are present with both Java
8 and Java 11.
- promoted test_jvm_mem_tracking to run in all strategies, as it's fast
and ensures JAVA_TOOL_OPTIONS is honored.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
---
M be/src/common/init.cc
M bin/run-all-tests.sh
M docker/daemon_entrypoint.sh
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 65 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/39/19939/7
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Restricts jvm_automatic_add_opens to only apply to Java 9+ where the
option exists. Previously it would also include it in Java 8, which
caused the JVM to ignore all options in JAVA_TOOL_OPTIONS.
Tests for Java version by running $JAVA_HOME/bin/java -version (or
"java" if JAVA_HOME is unset) and parsing version from the first line.
All JVM implementations are expected to include the version in a quoted
string, such as "1.8.0_42" and "11.0.1".
Also added add-opens flags for frontend tests.
test_no_inaccessible_objects detected this in a test run.
Testing:
- manually confirmed -agentlib options are present with both Java
8 and Java 11.
- promoted test_jvm_mem_tracking to run in all strategies, as it's fast
and ensures JAVA_TOOL_OPTIONS is honored.
Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Reviewed-on: http://gerrit.cloudera.org:8080/19939
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Reviewed-by: Quanlong Huang <hu...@gmail.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/init.cc
M bin/run-all-tests.sh
M docker/daemon_entrypoint.sh
M tests/custom_cluster/test_jvm_mem_tracking.py
4 files changed, 65 insertions(+), 8 deletions(-)
Approvals:
Zoltan Borok-Nagy: Looks good to me, but someone else must approve
Joe McDonnell: Looks good to me, but someone else must approve
Quanlong Huang: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13131/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 May 2023 19:12:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc
File be/src/common/init.cc:
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@315
PS2, Line 315: if (java_home != NULL) {
: cmd = (boost::filesystem::path(java_home) / "bin" / "java").string();
: }
> I added using PATH as a fallback because of the docker images.
It's good to support falling back to PATH, but we can also export JAVA_HOME in docker/daemon_entrypoint.sh.
http://gerrit.cloudera.org:8080/#/c/19939/2/be/src/common/init.cc@335
PS2, Line 335: if (std::stoi(matches.str(1)) < 9) return Status::OK();
> As in try/catch? I think a DCHECK also would, and we control the regex. I c
Right, with our regex there shouldn't be an issue. We could limit the number of digits we support for java version to 3. i.e. ([0-9]{1,3}), then there shouldn't be any condition that would produce an exception.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 May 2023 03:24:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7: Code-Review+1
This is looking good to me, and I'm willing to go to +2.
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 20:03:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19939 )
Change subject: IMPALA-11260: (Addendum) Restrict add-opens to Java 9+
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9369/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/19939
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I85953e685f6bbbd213afd93f389066e82f193ddf
Gerrit-Change-Number: 19939
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Jun 2023 23:36:13 +0000
Gerrit-HasComments: No