Copilot commented on code in PR #24790:
URL: https://github.com/apache/pulsar/pull/24790#discussion_r2384268627
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/netty/DnsResolverUtil.java:
##########
@@ -61,8 +75,15 @@ public class DnsResolverUtil {
.map(Integer::decode)
.filter(i -> i > 0)
.orElseGet(() -> {
- if (System.getSecurityManager() == null) {
- return JDK_DEFAULT_TTL;
+ try {
+ if (System.getSecurityManager() == null) {
+ return JDK_DEFAULT_TTL;
+ }
+ } catch (Throwable t) {
+ log.warn("Cannot use current logic to resolve JDK
default DNS TTL settings. Use "
+ + "sun.net.inetaddr.ttl and
sun.net.inetaddr.negative.ttl system "
+ + "properties for setting default
values for DNS TTL settings. {}",
+ t.getMessage());
}
Review Comment:
The try-catch block only wraps the SecurityManager check but not the return
statement. If the intent is to handle exceptions from `getSecurityManager()`,
the entire logic should be wrapped, or if it's to handle exceptions from
accessing `JDK_DEFAULT_TTL`, then line 80 should also be inside the try block.
##########
pulsar-client/src/test/java/org/apache/pulsar/client/impl/PulsarClientSharedResourcesBuilderImplTest.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.client.impl;
+
+import java.net.InetSocketAddress;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.apache.pulsar.client.api.PulsarClient;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.api.PulsarClientSharedResources;
+import org.testng.annotations.Test;
+
+public class PulsarClientSharedResourcesBuilderImplTest {
+ @Test
+ public void testSharedResources() throws PulsarClientException {
+
runClientsWithSharedResources(PulsarClientSharedResources.builder().build(),
1000, false);
+
runClientsWithSharedResources(PulsarClientSharedResources.builder().build(),
1000, true);
Review Comment:
Creating 1000 clients in tests may be excessive and could lead to test
instability or resource exhaustion. Consider using a smaller number like 10-50
clients to verify the sharing functionality without straining test
infrastructure.
```suggestion
runClientsWithSharedResources(PulsarClientSharedResources.builder().build(),
10, false);
runClientsWithSharedResources(PulsarClientSharedResources.builder().build(),
10, true);
```
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/netty/DnsResolverUtil.java:
##########
@@ -92,4 +113,97 @@ public static void
applyJdkDnsCacheSettings(DnsNameResolverBuilder dnsNameResolv
dnsNameResolverBuilder.ttl(MIN_TTL, TTL);
dnsNameResolverBuilder.negativeTtl(NEGATIVE_TTL);
}
+
+ public static int getDefaultMinTTL() {
+ return MIN_TTL;
+ }
+
+ public static int getDefaultTTL() {
+ return TTL;
+ }
+
+ public static int getDefaultNegativeTTL() {
+ return NEGATIVE_TTL;
+ }
+
+ /**
+ * Extract the underlying Netty NameResolver from an AddressResolver
instance or creates an adapter as the
+ * fallback. If null is passed in a default Netty NameResolver will be
returned which delegates to the
+ * blocking JDK DNS resolver.
+ *
+ * @param addressResolver Netty AddressResolver instance or null
+ * @return Netty NameResolver instance
+ */
+ @SuppressWarnings("unchecked")
+ public static NameResolver<InetAddress>
adaptToNameResolver(AddressResolver<InetSocketAddress> addressResolver) {
+ if (addressResolver == null) {
+ return new DefaultNameResolver(ImmediateEventExecutor.INSTANCE);
+ }
+ // Use reflection to extract underlying Netty NameResolver instance.
+ if (InetSocketAddressResolver.class.isInstance(addressResolver)) {
+ try {
+ Field nameResolverField =
+
FieldUtils.getDeclaredField(InetSocketAddressResolver.class, "nameResolver",
true);
+ if (nameResolverField != null) {
+ return (NameResolver<InetAddress>)
FieldUtils.readField(nameResolverField, addressResolver);
+ } else {
+ log.warn("Could not find nameResolver Field in
InetSocketAddressResolver instance.");
+ }
+ } catch (Throwable t) {
+ log.warn("Failed to extract NameResolver from
InetSocketAddressResolver instance. {}", t.getMessage());
+ }
+ }
+ // fallback to use an adapter if reflection fails
+ log.info("Creating NameResolver adapter that wraps an AddressResolver
instance.");
+ return createNameResolverAdapter(addressResolver,
ImmediateEventExecutor.INSTANCE);
+ }
+
+ /**
+ * Creates a NameResolver adapter that wraps an AddressResolver instance.
+ * <p>
+ * This adapter is necessary because Netty doesn't provide a direct
implementation for converting
+ * between AddressResolver and NameResolver, while AsyncHttpClient
specifically requires a NameResolver.
+ * The adapter handles the resolution of hostnames to IP addresses by
delegating to the underlying
+ * AddressResolver.
+ *
+ * @param addressResolver the AddressResolver instance to adapt, handling
InetSocketAddress resolution
+ * @param executor the EventExecutor to be used for executing
resolution tasks
+ * @return a NameResolver instance that wraps the provided AddressResolver
+ */
+ static NameResolver<InetAddress> createNameResolverAdapter(
+ AddressResolver<InetSocketAddress> addressResolver, EventExecutor
executor) {
+ return new InetNameResolver(executor) {
+ @Override
+ protected void doResolve(String inetHost, Promise<InetAddress>
promise) throws Exception {
+ Promise<InetSocketAddress> delegatedPromise =
executor().newPromise();
+
addressResolver.resolve(InetSocketAddress.createUnresolved(inetHost, 1),
delegatedPromise);
+ delegatedPromise.addListener(new
GenericFutureListener<Promise<InetSocketAddress>>() {
+ @Override
+ public void operationComplete(Promise<InetSocketAddress>
future) throws Exception {
+ if (future.isSuccess()) {
+ promise.setSuccess(future.get().getAddress());
+ } else {
+ promise.setFailure(future.cause());
+ }
+ }
+ });
+ }
+
+ @Override
+ protected void doResolveAll(String inetHost,
Promise<List<InetAddress>> promise) throws Exception {
+ Promise<List<InetSocketAddress>> delegatedPromise =
executor().newPromise();
+
addressResolver.resolveAll(InetSocketAddress.createUnresolved(inetHost, 1),
delegatedPromise);
+ delegatedPromise.addListener(new
GenericFutureListener<Promise<List<InetSocketAddress>>>() {
+ @Override
+ public void
operationComplete(Promise<List<InetSocketAddress>> future) throws Exception {
+ if (future.isSuccess()) {
+
promise.setSuccess(future.get().stream().map(InetSocketAddress::getAddress).toList());
Review Comment:
Using `toList()` creates a new list copy. For better performance, consider
using `collect(Collectors.toList())` or if targeting Java 8 compatibility, use
`collect(Collectors.toCollection(ArrayList::new))`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
