-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][cli] Fulfill add-opens to function-localrunner also #20417
Conversation
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
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" |
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.
This is the effective change. Others are refactors during the debugging stage.
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. |
@tisonkun thanks for this fix. and i saw more add-open options in pulsar start script. do we need them also ? see Lines 297 to 311 in aa3bfcd
|
@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 { |
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.
please remove this change that seems unrelated
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.
Overall I support the patch, but there is some change that seems unrelated
This reverts commit 60ad75a.
@eolivelli reverted |
Confirmed with @RobertIndie . Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
This closes #20282.
Motivation
Fulfill add-opens to function-localrunner also.
This issue affects version >= 2.11.
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: