You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/11 07:40:37 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #15540: [Fix][broker] Fix JDK17 compatibility issues.
mattisonchao opened a new pull request, #15540:
URL: https://github.com/apache/pulsar/pull/15540
Fixes #15539
Master Issue: #15539
### Motivation
See #15539
### Modifications
- Add `--add-opens java.base/sun.net=ALL-UNNAMED`
- Add `--add-opens java.management/sun.management=ALL-UNNAMED`
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1124189890
> Thank you for taking care of this issue. I am wondering if removing reflection could resolve the issue too. Adding `--add-opens` seems radical unless required.
The benefit of the reflection based solution is that it's possible to target Java 8 with `--release 8` (`maven.compiler.release`) when compiling with Java 17. Since DnsResolverUtil must support targeting Java 8, I don't think we have another option than using the `--add-opens` solution for DnsResolverUtil.
IIRC, `--add-exports` will be needed at compile time when reflection isn't used and I don't think that is compatible with `--release 8`. That's the reason for the use of reflection in both of these cases.
It might be better to get rid of the sun.net.InetAddressPolicy class usage and just duplicate the logic in DnsResolverUtil.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1124298048
> @heesung-sn The benefit of the reflection based solution is that it's possible to target Java 8 with `--release 8` (`maven.compiler.release`) when compiling with Java 17. Since DnsResolverUtil must support targeting Java 8, I don't think we have another option than using the `--add-opens` solution for DnsResolverUtil.
>
> IIRC, `--add-exports` will be needed at compile time when reflection isn't used and I don't think that is compatible with `--release 8`. That's the reason for the use of reflection in both of these cases.
>
> It might be better to get rid of the sun.net.InetAddressPolicy class usage and just duplicate the logic in DnsResolverUtil.
Ack. If there is no other option, I am ok with the `--add-opens` solution. Thank you for clarification.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] heesung-sn commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
heesung-sn commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1123354744
Thank you for taking care of this issue. I am wondering if removing reflection could resolve the issue too. Adding `--add-opens` seems radical unless required.
For example, can we directly use the static functions in `InetAddressCachePolicy` for this code?, https://github.com/openjdk/jdk17/blob/master/src/java.base/share/classes/sun/net/InetAddressCachePolicy.java#L154-L160
```
static {
int ttl = DEFAULT_TTL;
int negativeTtl = DEFAULT_NEGATIVE_TTL;
try {
// use reflection to call sun.net.InetAddressCachePolicy's get and getNegative methods for getting
// effective JDK settings for DNS caching
Class<?> inetAddressCachePolicyClass = Class.forName("sun.net.InetAddressCachePolicy");
Method getTTLMethod = inetAddressCachePolicyClass.getMethod("get");
ttl = (Integer) getTTLMethod.invoke(null);
Method getNegativeTTLMethod = inetAddressCachePolicyClass.getMethod("getNegative");
negativeTtl = (Integer) getNegativeTTLMethod.invoke(null);
} catch (NoSuchMethodException | ClassNotFoundException | InvocationTargetException
| IllegalAccessException e) {
log.warn("Cannot get DNS TTL settings from sun.net.InetAddressCachePolicy class", e);
}
TTL = useDefaultTTLWhenSetToForever(ttl, DEFAULT_TTL);
NEGATIVE_TTL = useDefaultTTLWhenSetToForever(negativeTtl, DEFAULT_NEGATIVE_TTL);
}
```
Similarly for this code path, we can use the static functions in `ManagementFactoryHelper`
https://github.com/openjdk/jdk17/blob/master/src/java.management/share/classes/sun/management/ManagementFactoryHelper.java
```
private static Method getTotalSafepointTimeHandle;
private static Method getSafepointCountHandle;
private Map<String, GCMetrics> gcMetricsMap = new HashMap<>();
static {
try {
runtime = Class.forName("sun.management.ManagementFactoryHelper")
.getMethod("getHotspotRuntimeMBean")
.invoke(null);
getTotalSafepointTimeHandle = runtime.getClass().getMethod("getTotalSafepointTime");
getTotalSafepointTimeHandle.setAccessible(true);
getSafepointCountHandle = runtime.getClass().getMethod("getSafepointCount");
getSafepointCountHandle.setAccessible(true);
// try to use the methods
getTotalSafepointTimeHandle.invoke(runtime);
getSafepointCountHandle.invoke(runtime);
} catch (Throwable e) {
log.warn("Failed to get Runtime bean", e);
}
}
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1124176613
#15349 is about DnsResolverUtil
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1193516578
@mattisonchao Please help create a PR for branch-2.10, lots of conflicts when I cherry-picking this PR.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1231462497
> @mattisonchao Please help create a PR for branch-2.10, lots of conflicts when I cherry-picking this PR.
+1 @mattisonchao
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liangyepianzhou commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1445018879
>@mattisonchao Please help create a PR for branch-2.10, lots of conflicts when I cherry-picking this PR.
+1 @mattisonchao I am cherry-picking this PR to branch-2.10. But I found the place of this PR modified does not exist in branch-2.10.
Could you please take a look at whether this Pr needs to cherry-pick to branch-2.10?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1123298341
@heesung-sn
Please take a look, thanks.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15540:
URL: https://github.com/apache/pulsar/pull/15540#issuecomment-1123376182
@heesung-sn
I see, thanks for your explanation, I will try to another way solve this problem.
Convert this PR to draft.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #15540: [Fix][broker] Fix JDK17 compatibility issues.
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #15540:
URL: https://github.com/apache/pulsar/pull/15540
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org