Skip to content

[fix][cli] Fulfill add-opens to function-localrunner also#20417

Merged
tisonkun merged 4 commits into
apache:masterfrom
tisonkun:issue-20282-tk2
May 31, 2023
Merged

[fix][cli] Fulfill add-opens to function-localrunner also#20417
tisonkun merged 4 commits into
apache:masterfrom
tisonkun:issue-20282-tk2

Conversation

@tisonkun

Copy link
Copy Markdown
Member

This closes #20282.

Motivation

Fulfill add-opens to function-localrunner also.

This issue affects version >= 2.11.

Verifying this change

  • Make sure that the change passes the CI checks and manually tested locally.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

tisonkun added 3 commits May 28, 2023 09:15
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun added this to the 3.1.0 milestone May 28, 2023
@tisonkun tisonkun requested review from Technoboy- and lhotari May 28, 2023 01:41
@tisonkun tisonkun self-assigned this May 28, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 28, 2023
Comment thread bin/function-localrunner
# >= JDK 9
PULSAR_GC_LOG=${PULSAR_GC_LOG:-"-Xlog:gc:logs/pulsar_gc_%p.log:time,uptime:filecount=10,filesize=20M"}
# '--add-opens' option is not supported in JDK 1.8
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED"

@tisonkun tisonkun May 28, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the effective change. Others are refactors during the debugging stage.

@codecov-commenter

codecov-commenter commented May 28, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.90%. Comparing base (aa3bfcd) to head (34077e6).
⚠️ Report is 1966 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20417      +/-   ##
============================================
- Coverage     72.90%   72.90%   -0.01%     
- Complexity    31864    31870       +6     
============================================
  Files          1864     1864              
  Lines        138416   138416              
  Branches      15188    15188              
============================================
- Hits         100919   100918       -1     
- Misses        29477    29479       +2     
+ Partials       8020     8019       -1     
Flag Coverage Δ
inttests 24.16% <ø> (-0.01%) ⬇️
systests 25.04% <ø> (-0.02%) ⬇️
unittests 72.18% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 75 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lifepuzzlefun

Copy link
Copy Markdown
Contributor

@tisonkun thanks for this fix. and i saw more add-open options in pulsar start script. do we need them also ? see

pulsar/bin/pulsar

Lines 297 to 311 in aa3bfcd

if [[ -z "$IS_JAVA_8" ]]; then
# BookKeeper: enable posix_fadvise usage and DirectMemoryCRC32Digest (https://github.com/apache/bookkeeper/pull/3234)
OPTS="$OPTS --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util.zip=ALL-UNNAMED"
# Netty: enable java.nio.DirectByteBuffer
# https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/PlatformDependent0.java
# https://github.com/netty/netty/issues/12265
OPTS="$OPTS --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED"
# netty.DnsResolverUtil
OPTS="$OPTS --add-opens java.base/sun.net=ALL-UNNAMED"
# JvmDefaultGCMetricsLogger & MBeanStatsGenerator
OPTS="$OPTS --add-opens java.management/sun.management=ALL-UNNAMED"
# MBeanStatsGenerator
OPTS="$OPTS --add-opens jdk.management/com.sun.management.internal=ALL-UNNAMED"
# LinuxInfoUtils
OPTS="$OPTS --add-opens java.base/jdk.internal.platform=ALL-UNNAMED"

@tisonkun

Copy link
Copy Markdown
Member Author

@lifepuzzlefun I tend to add them on demand instead of pull in too much options. This is also how the upstream JDK's trend to restrict access.

That said, if we open unnecessary modules, it's more likely we introduce more dependency without explicit review.

@lifepuzzlefun

Copy link
Copy Markdown
Contributor

@lifepuzzlefun I tend to add them on demand instead of pull in too much options. This is also how the upstream JDK's trend to restrict access.

That said, if we open unnecessary modules, it's more likely we introduce more dependency without explicit review.

yes, i agree with you.

public void testMaxTtl() {
EventLoop eventLoop = Mockito.mock(EventLoop.class);
DnsNameResolverBuilder dnsNameResolverBuilder = new DnsNameResolverBuilder(eventLoop);
public void testMaxTtl() throws Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove this change that seems unrelated

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall I support the patch, but there is some change that seems unrelated

@tisonkun tisonkun requested a review from eolivelli May 29, 2023 01:11
@tisonkun

Copy link
Copy Markdown
Member Author

@eolivelli reverted

@tisonkun tisonkun merged commit da2a148 into apache:master May 31, 2023
@tisonkun tisonkun deleted the issue-20282-tk2 branch May 31, 2023 01:49
tisonkun added a commit that referenced this pull request Jun 7, 2023
Confirmed with @RobertIndie .

Signed-off-by: tison <wander4096@gmail.com>
Technoboy- pushed a commit that referenced this pull request Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] DnsResolverUtil "Cannot get DNS TTL settings from sun.net.InetAddressCachePolicy class" in Java 17

6 participants