absurdfarce commented on code in PR #2013: URL: https://github.com/apache/cassandra-java-driver/pull/2013#discussion_r1999682162
########## core/src/main/java/com/datastax/oss/driver/internal/core/addresstranslation/SubnetAddressTranslator.java: ########## @@ -0,0 +1,238 @@ +/* + * 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 com.datastax.oss.driver.internal.core.addresstranslation; + +import com.datastax.oss.driver.api.core.addresstranslation.AddressTranslator; +import com.datastax.oss.driver.api.core.config.DriverOption; +import com.datastax.oss.driver.api.core.context.DriverContext; +import com.datastax.oss.driver.internal.core.util.AddressUtils; +import edu.umd.cs.findbugs.annotations.NonNull; +import edu.umd.cs.findbugs.annotations.Nullable; +import inet.ipaddr.IPAddress; +import inet.ipaddr.IPAddressString; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This translator returns the proxy address of the private subnet containing the Cassandra node IP, + * or default address if no matching subnets, or passes through the original node address if no + * default configured. + * + * <p>The translator can be used for scenarios when all nodes are behind some kind of proxy, and + * that proxy is different for nodes located in different subnets (eg. when Cassandra is deployed in + * multiple datacenters/regions). One can use this, for example, for Cassandra on Kubernetes with + * different Cassandra datacenters deployed to different Kubernetes clusters. + */ +public class SubnetAddressTranslator implements AddressTranslator { + private static final Logger LOG = LoggerFactory.getLogger(SubnetAddressTranslator.class); + + /** + * A map of Cassandra node subnets (CIDR notations) to target addresses, for example (note quoted + * keys): + * + * <pre> + * advanced.address-translator.subnet-addresses { + * "100.64.0.0/15" = "cassandra.datacenter1.com:9042" + * "100.66.0.0/15" = "cassandra.datacenter2.com:9042" + * # IPv6 example: + * # "::ffff:6440:0/111" = "cassandra.datacenter1.com:9042" + * # "::ffff:6442:0/111" = "cassandra.datacenter2.com:9042" + * } + * </pre> + * + * Note: subnets must be represented as prefix blocks, see {@link + * inet.ipaddr.Address#isPrefixBlock()}. + */ + public static final String ADDRESS_TRANSLATOR_SUBNET_ADDRESSES = + "advanced.address-translator.subnet-addresses"; + + /** + * A default address to fallback to if Cassandra node IP isn't contained in any of the configured + * subnets. + */ + public static final String ADDRESS_TRANSLATOR_DEFAULT_ADDRESS = + "advanced.address-translator.default-address"; + + /** + * Whether to resolve the addresses on initialization (if true) or on each node (re-)connection + * (if false). Defaults to false. + */ + public static final String ADDRESS_TRANSLATOR_RESOLVE_ADDRESSES = + "advanced.address-translator.resolve-addresses"; + + public static DriverOption ADDRESS_TRANSLATOR_SUBNET_ADDRESSES_OPTION = + new DriverOption() { + @NonNull + @Override + public String getPath() { + return ADDRESS_TRANSLATOR_SUBNET_ADDRESSES; + } + }; + + public static DriverOption ADDRESS_TRANSLATOR_DEFAULT_ADDRESS_OPTION = + new DriverOption() { + @NonNull + @Override + public String getPath() { + return ADDRESS_TRANSLATOR_DEFAULT_ADDRESS; + } + }; + + public static DriverOption ADDRESS_TRANSLATOR_RESOLVE_ADDRESSES_OPTION = + new DriverOption() { + @NonNull + @Override + public String getPath() { + return ADDRESS_TRANSLATOR_RESOLVE_ADDRESSES; + } + }; Review Comment: These DriverOptions need to be moved to DefaultDriverOption. We try to keep all the options for the basic driver there even if they're only enabled in certain cases. Note that the options for Astra connectivity [are also included there](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java#L797-L803) even though not all users of the Java driver make use of Astra. I'm guessing this usage was derived from Stefan's [example](https://github.com/apache/cassandra-java-driver/blob/7982f413a90935a91549aa3ee7cc64fa25d7c113/core/src/main/java/com/datastax/oss/driver/internal/core/addresstranslation/FixedHostNameAddressTranslator.java#L43-L50) in FixedHostNameAddressTranslator. We need to clean that one up as well. Also note that you'll want to add the new values to the DefaultDriverOption enum at the end. If you don't you'll get complaints from some of the build checks about potential disruptions to ordinal-based access of the enum. See [this commit](https://github.com/apache/cassandra-java-driver/commit/7982f413a90935a91549aa3ee7cc64fa25d7c113) for more context. ########## core/pom.xml: ########## @@ -116,6 +116,11 @@ <groupId>org.reactivestreams</groupId> <artifactId>reactive-streams</artifactId> </dependency> + <dependency> + <groupId>com.github.seancfoley</groupId> + <artifactId>ipaddress</artifactId> + <optional>true</optional> + </dependency> Review Comment: I really don't love the inclusion of _another_ dependency here, even if it's an optional one. It's only used in one class (near as I can tell)... is there really no way to get the functionality we need without adding this in? ########## core/src/main/java/com/datastax/oss/driver/internal/core/ContactPoints.java: ########## @@ -41,7 +38,22 @@ public static Set<EndPoint> merge( Set<EndPoint> result = Sets.newHashSet(programmaticContactPoints); for (String spec : configContactPoints) { - for (InetSocketAddress address : extract(spec, resolve)) { + + Set<InetSocketAddress> addresses = Collections.emptySet(); + try { + addresses = AddressUtils.extract(spec, resolve); + } catch (RuntimeException e) { + LOG.warn("Ignoring invalid contact point {} ({})", spec, e.getMessage(), e); + } Review Comment: As I look at this now it feels like we had to make this a bit more complicated because AddressUtils.extract() now throws exceptions in most cases rather than just logging errors and returning an empty set. Was there a particular reason for this change? It's not immediately clear the exceptions buy you much here. ########## core/src/main/java/com/datastax/oss/driver/internal/core/ContactPoints.java: ########## @@ -41,7 +38,22 @@ public static Set<EndPoint> merge( Set<EndPoint> result = Sets.newHashSet(programmaticContactPoints); for (String spec : configContactPoints) { - for (InetSocketAddress address : extract(spec, resolve)) { + + Set<InetSocketAddress> addresses = Collections.emptySet(); + try { + addresses = AddressUtils.extract(spec, resolve); + } catch (RuntimeException e) { + LOG.warn("Ignoring invalid contact point {} ({})", spec, e.getMessage(), e); + } + + if (addresses.size() > 1) { + LOG.info( + "Contact point {} resolves to multiple addresses, will use them all ({})", + spec, + addresses); + } Review Comment: Does this log message offer us much useful information? ########## core/src/main/java/com/datastax/oss/driver/internal/core/ContactPoints.java: ########## @@ -41,7 +38,22 @@ public static Set<EndPoint> merge( Set<EndPoint> result = Sets.newHashSet(programmaticContactPoints); for (String spec : configContactPoints) { - for (InetSocketAddress address : extract(spec, resolve)) { + + Set<InetSocketAddress> addresses = Collections.emptySet(); + try { + addresses = AddressUtils.extract(spec, resolve); + } catch (RuntimeException e) { + LOG.warn("Ignoring invalid contact point {} ({})", spec, e.getMessage(), e); + } Review Comment: Could just continue here to next iteration of the outer for loop. You know addresses is the empty set at this point so there's no point iterating over it below. -- 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: pr-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org For additional commands, e-mail: pr-h...@cassandra.apache.org
