Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@
<optional>true</optional>
</dependency>

<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-resolver-dns</artifactId>
<optional>true</optional>
</dependency>

<!-- OS-native transports -->

<dependency>
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/io/lettuce/core/AbstractRedisClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
* @author Mark Paluch
* @author Jongyeol Choi
* @author Poorva Gokhale
* @author Yohei Ueki
* @since 3.0
* @see ClientResources
*/
Expand Down Expand Up @@ -297,6 +298,15 @@ protected void channelType(ConnectionBuilder connectionBuilder, ConnectionPoint
}
}

protected void resolver(ConnectionBuilder connectionBuilder, ConnectionPoint connectionPoint) {

LettuceAssert.notNull(connectionPoint, "ConnectionPoint must not be null");

if (connectionPoint.getSocket() == null) {
connectionBuilder.bootstrap().resolver(clientResources.addressResolverGroup());
}
}

private EventLoopGroup getEventLoopGroup(ConnectionPoint connectionPoint) {

for (;;) {
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/io/lettuce/core/RedisClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
*
* @author Will Glozer
* @author Mark Paluch
* @author Yohei Ueki
* @see RedisURI
* @see StatefulRedisConnection
* @see RedisFuture
Expand Down Expand Up @@ -322,6 +323,7 @@ private <K, V, S> ConnectionFuture<S> connectStatefulAsync(StatefulRedisConnecti
connectionBuilder(getSocketAddressSupplier(redisURI), connectionBuilder, redisURI);
connectionBuilder.connectionInitializer(createHandshake(state));
channelType(connectionBuilder, redisURI);
resolver(connectionBuilder, redisURI);
Comment on lines 325 to +326
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Supplemental Comments]
I added resolver(...) all places where channelType(...) is used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to refactor the connect code a bit to pull things more together. I'll take care of this during the merge.


ConnectionFuture<RedisChannelHandler<K, V>> future = initializeChannelAsync(connectionBuilder);

Expand Down Expand Up @@ -599,6 +601,7 @@ private <K, V> ConnectionFuture<StatefulRedisSentinelConnection<K, V>> doConnect
connectionBuilder(getSocketAddressSupplier(redisURI), connectionBuilder, redisURI);

channelType(connectionBuilder, redisURI);
resolver(connectionBuilder, redisURI);
ConnectionFuture<?> sync = initializeChannelAsync(connectionBuilder);

return sync.thenApply(ignore -> (StatefulRedisSentinelConnection<K, V>) connection).whenComplete((ignore, e) -> {
Expand Down
26 changes: 24 additions & 2 deletions src/main/java/io/lettuce/core/Transports.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,19 @@
import io.netty.channel.Channel;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.socket.DatagramChannel;
import io.netty.channel.socket.nio.NioDatagramChannel;
import io.netty.channel.socket.nio.NioSocketChannel;

/**
* Transport infrastructure utility class. This class provides {@link EventLoopGroup} and {@link Channel} classes for socket and
* native socket transports.
*
* @author Mark Paluch
* @author Yohei Ueki
* @since 4.4
*/
class Transports {
public class Transports {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Supplemental Comments]
I made this public because this is now referenced by outside of the package.
ec85577#diff-724688b7234bd44bd8e5f08ce8b0a616ecaf11729ebd99ad72d510431878a6f4R60-R63

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should move Transports to the resource package, that's something we can do during the merge.


/**
* @return the default {@link EventLoopGroup} for socket transport that is compatible with {@link #socketChannelClass()}.
Expand All @@ -48,7 +51,7 @@ static Class<? extends EventLoopGroup> eventLoopGroupClass() {
/**
* @return the default {@link Channel} for socket (network/TCP) transport.
*/
static Class<? extends Channel> socketChannelClass() {
public static Class<? extends Channel> socketChannelClass() {

if (NativeTransports.isSocketSupported()) {
return NativeTransports.socketChannelClass();
Expand All @@ -57,6 +60,18 @@ static Class<? extends Channel> socketChannelClass() {
return NioSocketChannel.class;
}

/**
* @return the default {@link DatagramChannel} for socket (network/UDP) transport.
*/
public static Class<? extends DatagramChannel> datagramChannelClass() {

if (NativeTransports.isSocketSupported()) {
return NativeTransports.datagramChannelClass();
}

return NioDatagramChannel.class;
}

/**
* Native transport support.
*/
Expand All @@ -79,6 +94,13 @@ static Class<? extends Channel> socketChannelClass() {
return RESOURCES.socketChannelClass();
}

/**
* @return the native transport socket {@link DatagramChannel} class.
*/
static Class<? extends DatagramChannel> datagramChannelClass() {
return RESOURCES.datagramChannelClass();
}

/**
* @return the native transport domain socket {@link Channel} class.
*/
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/io/lettuce/core/cluster/RedisClusterClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@
* possible.
*
* @author Mark Paluch
* @author Yohei Ueki
* @since 3.0
* @see RedisURI
* @see StatefulRedisClusterConnection
Expand Down Expand Up @@ -815,6 +816,7 @@ private <K, V> ConnectionBuilder createConnectionBuilder(RedisChannelHandler<K,
connectionBuilder.commandHandler(commandHandlerSupplier);
connectionBuilder(socketAddressSupplier, connectionBuilder, connectionSettings);
channelType(connectionBuilder, connectionSettings);
resolver(connectionBuilder, connectionSettings);

return connectionBuilder;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package io.lettuce.core.resource;

import java.util.function.Supplier;

import io.lettuce.core.Transports;
import io.netty.channel.socket.SocketChannel;
import io.netty.resolver.AddressResolverGroup;
import io.netty.resolver.DefaultAddressResolverGroup;
import io.netty.resolver.dns.DnsAddressResolverGroup;
import io.netty.resolver.dns.DnsNameResolverBuilder;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;

/**
* Wraps and provides {@link AddressResolverGroup} classes. This is to protect the user from {@link ClassNotFoundException}'s
* caused by the absence of the {@literal netty-dns-resolver} library during runtime. This class will be deleted when
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Supplemental Comments]
This class can be deleted when netty-dns-resolver becomes mandatory: simply replace here with
https://github.com/lettuce-io/lettuce-core/pull/1517/files#diff-596af0e5ee58e7de4607c1c26b0ab4bfad308c0f2a713d52caa5a68a2aa1cf9cR112-R113

    public static final AddressResolverGroup<?> DEFAULT_ADDRESS_RESOLVER_GROUP = new DnsAddressResolverGroup(new DnsNameResolverBuilder().channelType(Transports.datagramChannelClass())
                .socketChannelType(Transports.socketChannelClass().asSubclass(SocketChannel.class)));

* {@literal netty-dns-resolver} becomes mandatory. Internal API.
*
* @author Yohei Ueki
* @since xxx
*/
class AddressResolverGroupProvider {

private static final InternalLogger logger = InternalLoggerFactory.getInstance(AddressResolverGroupProvider.class);

private static final AddressResolverGroup<?> ADDRESS_RESOLVER_GROUP;

static {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be unit-tested, but I could not come up with a good way to write unit tests regarding this in lettuce's test repository, because we need unittest jars with and without netty-dns-resolver. We might be able do it by dirty-hacking class loader, but I am not quite familiar with it.

I wrote a unit test code outside of this repo:
https://github.com/yueki1993/lettuce-test-PR1517/blob/main/lettuce-test-PR1517-without-netty-dns-resolver/src/test/java/com/github/yueki1993/lettuce_test_PR1517/without_netty_dns_resolver/AddressResolverGroupTests.java
https://github.com/yueki1993/lettuce-test-PR1517/blob/main/lettuce-test-PR1517-with-netty-dns-resolver/src/test/java/com/github/yueki1993/lettuce_test_PR1517/with_netty_dns_resolver/AddressResolverGroupTests.java

boolean dnsResolverAvailable;
try {
Class.forName("io.netty.resolver.dns.DnsAddressResolverGroup");
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use LettuceClassUtils.isPresent(…).

dnsResolverAvailable = true;
} catch (ClassNotFoundException e) {
dnsResolverAvailable = false;
}

// create addressResolverGroup instance via Supplier to avoid NoClassDefFoundError.
Copy link
Contributor Author

@yueki1993 yueki1993 Nov 22, 2020

Choose a reason for hiding this comment

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

[Supplemental Comments]
Actually I am not so familiar with jvm's classloader.
I found developer docs of jenkins, and they say using generics types will not raise NoClassDefFoundError.
Thus I wrapped this by Supplier<AddressResolverGroup<?>> and created AddressResolverGroup<?> instance via supplier.
https://www.jenkins.io/doc/developer/plugin-development/optional-dependencies/

Supplier<AddressResolverGroup<?>> supplier;
if (dnsResolverAvailable) {
logger.debug("Starting with netty's non-blocking DNS resolver library");
supplier = AddressResolverGroupProvider::defaultDnsAddressResolverGroup;
} else {
logger.debug("Starting without optional netty's non-blocking DNS resolver library");
supplier = () -> DefaultAddressResolverGroup.INSTANCE;
}
ADDRESS_RESOLVER_GROUP = supplier.get();
}

/**
* Returns the {@link AddressResolverGroup} for dns resolution.
*
* @return the {@link DnsAddressResolverGroup} if {@literal netty-dns-resolver} is available, otherwise return
* {@link DefaultAddressResolverGroup#INSTANCE}.
* @since xxx
*/
static AddressResolverGroup<?> addressResolverGroup() {
return ADDRESS_RESOLVER_GROUP;
}

private static DnsAddressResolverGroup defaultDnsAddressResolverGroup() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method signature creates a dependency to DnsAddressResolverGroup that the class loader tries to resolve when the class gets loaded. Please inline this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I tried with my UT and jenkins' doc, this did not produce ClassNotFoundError or something, because it is declared as private and it is not upcasted.
But i don't have strong reason to stick with this, so this also leave to you.

return new DnsAddressResolverGroup(new DnsNameResolverBuilder().channelType(Transports.datagramChannelClass())
.socketChannelType(Transports.socketChannelClass().asSubclass(SocketChannel.class)));
}

}
23 changes: 23 additions & 0 deletions src/main/java/io/lettuce/core/resource/ClientResources.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import io.lettuce.core.metrics.CommandLatencyCollectorOptions;
import io.lettuce.core.metrics.CommandLatencyRecorder;
import io.lettuce.core.tracing.Tracing;
import io.netty.resolver.AddressResolverGroup;
import io.netty.util.Timer;
import io.netty.util.concurrent.EventExecutorGroup;
import io.netty.util.concurrent.Future;
Expand All @@ -45,10 +46,12 @@
* <li>{@link DnsResolver} to collect latency details. Requires the {@literal LatencyUtils} library.</li>
* <li>{@link Timer} for scheduling</li>
* <li>{@link Tracing} to trace Redis commands.</li>
* <li>{@link AddressResolverGroup} for dns resolution.</li>
* </ul>
*
* @author Mark Paluch
* @author Mikhael Sokolov
* @author Yohei Ueki
* @since 3.4
* @see DefaultClientResources
*/
Expand Down Expand Up @@ -241,6 +244,18 @@ default Builder commandLatencyCollector(CommandLatencyCollector commandLatencyCo
*/
Builder tracing(Tracing tracing);

/**
* Sets the {@link AddressResolverGroup} for dns resolution. This option is only effective if
* {@link DnsResolvers#UNRESOLVED} is used as {@link DnsResolver}. Defaults to
* {@link io.netty.resolver.DefaultAddressResolverGroup#INSTANCE} if {@literal netty-dns-resolver} is not available,
* otherwise defaults to {@link io.netty.resolver.dns.DnsAddressResolverGroup}.
*
* @param addressResolverGroup the {@link AddressResolverGroup} instance, must not be {@code null}.
* @return {@code this} {@link Builder}
* @since xxx
Copy link
Collaborator

Choose a reason for hiding this comment

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

since 6.1

*/
Builder addressResolverGroup(AddressResolverGroup<?> addressResolverGroup);

/**
* @return a new instance of {@link DefaultClientResources}.
*/
Expand Down Expand Up @@ -385,4 +400,12 @@ default Builder commandLatencyCollector(CommandLatencyCollector commandLatencyCo
*/
Tracing tracing();

/**
* Return the {@link AddressResolverGroup} instance for dns resolution.
*
* @return the address resolver group.
* @since xxx
*/
AddressResolverGroup<?> addressResolverGroup();

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import io.lettuce.core.metrics.MetricCollector;
import io.lettuce.core.resource.Delay.StatefulDelay;
import io.lettuce.core.tracing.Tracing;
import io.netty.resolver.AddressResolverGroup;
import io.netty.util.HashedWheelTimer;
import io.netty.util.Timer;
import io.netty.util.concurrent.DefaultEventExecutorGroup;
Expand Down Expand Up @@ -70,9 +71,11 @@
* <li>a {@code socketAddressResolver} which is a provided instance of {@link SocketAddressResolver}.</li>
* <li>a {@code timer} that is a provided instance of {@link io.netty.util.HashedWheelTimer}.</li>
* <li>a {@code tracing} that is a provided instance of {@link Tracing}.</li>
* <li>a {@code addressResolverGroup} that is a provided instance of {@link AddressResolverGroup}.</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We try to sort elements alphabetically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(nits) also i noticed it should be an {@code addressResolverGroup} ...

* </ul>
*
* @author Mark Paluch
* @author Yohei Ueki
* @since 3.4
*/
public class DefaultClientResources implements ClientResources {
Expand Down Expand Up @@ -103,6 +106,12 @@ public class DefaultClientResources implements ClientResources {
*/
public static final NettyCustomizer DEFAULT_NETTY_CUSTOMIZER = DefaultNettyCustomizer.INSTANCE;

/**
* Default {@link AddressResolverGroup}.
*/
public static final AddressResolverGroup<?> DEFAULT_ADDRESS_RESOLVER_GROUP = AddressResolverGroupProvider
.addressResolverGroup();

static {

int threads = Math.max(1, SystemPropertyUtil.getInt("io.netty.eventLoopThreads",
Expand Down Expand Up @@ -147,6 +156,8 @@ public class DefaultClientResources implements ClientResources {

private final Tracing tracing;

private final AddressResolverGroup<?> addressResolverGroup;

private volatile boolean shutdownCalled = false;

protected DefaultClientResources(Builder builder) {
Expand Down Expand Up @@ -243,6 +254,7 @@ protected DefaultClientResources(Builder builder) {
reconnectDelay = builder.reconnectDelay;
nettyCustomizer = builder.nettyCustomizer;
tracing = builder.tracing;
addressResolverGroup = builder.addressResolverGroup;

if (!sharedTimer && timer instanceof HashedWheelTimer) {
((HashedWheelTimer) timer).start();
Expand Down Expand Up @@ -308,6 +320,8 @@ public static class Builder implements ClientResources.Builder {

private Tracing tracing = Tracing.disabled();

private AddressResolverGroup<?> addressResolverGroup = DEFAULT_ADDRESS_RESOLVER_GROUP;

private Builder() {
}

Expand Down Expand Up @@ -569,6 +583,25 @@ public Builder tracing(Tracing tracing) {
return this;
}

/**
* Sets the {@link AddressResolverGroup} for dns resolution. This option is only effective if
* {@link DnsResolvers#UNRESOLVED} is used as {@link DnsResolver}. Defaults to
* {@link io.netty.resolver.DefaultAddressResolverGroup#INSTANCE} if {@literal netty-dns-resolver} is not available,
* otherwise defaults to {@link io.netty.resolver.dns.DnsAddressResolverGroup}.
*
* @param addressResolverGroup the {@link AddressResolverGroup} instance, must not be {@code null}.
* @return {@code this} {@link ClientResources.Builder}
* @since xxx
Copy link
Collaborator

Choose a reason for hiding this comment

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

since 6.1

*/
@Override
public Builder addressResolverGroup(AddressResolverGroup<?> addressResolverGroup) {

LettuceAssert.notNull(addressResolverGroup, "AddressResolverGroup must not be null");

this.addressResolverGroup = addressResolverGroup;
return this;
}

/**
* @return a new instance of {@link DefaultClientResources}.
*/
Expand Down Expand Up @@ -603,7 +636,7 @@ public DefaultClientResources.Builder mutate() {
.commandLatencyPublisherOptions(commandLatencyPublisherOptions()).dnsResolver(dnsResolver())
.eventBus(eventBus()).eventExecutorGroup(eventExecutorGroup()).reconnectDelay(reconnectDelay)
.socketAddressResolver(socketAddressResolver()).nettyCustomizer(nettyCustomizer()).timer(timer())
.tracing(tracing());
.tracing(tracing()).addressResolverGroup(addressResolverGroup());

builder.sharedCommandLatencyCollector = sharedEventLoopGroupProvider;
builder.sharedEventExecutor = sharedEventExecutor;
Expand Down Expand Up @@ -742,4 +775,9 @@ public Tracing tracing() {
return tracing;
}

@Override
public AddressResolverGroup<?> addressResolverGroup() {
return addressResolverGroup;
}

}
Loading