absurdfarce commented on code in PR #2036: URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2116840068
########## core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.api.core.loadbalancing; + +import edu.umd.cs.findbugs.annotations.Nullable; + +/** Load balancing policy taking into account local datacenter of the application. */ +public interface LocalDcAwareLoadBalancingPolicy extends LoadBalancingPolicy { + + /** Returns the local datacenter name, if known; empty otherwise. */ + @Nullable + String getLocalDatacenter(); +} Review Comment: Oooooh, I like this very much... being explicit about whether an LBP cares about this (and building that into the type system) seems desirable. Note that we're not saying any specific LBP will take action in a particular way based on this information. Presumably all we can say of an LBP that implements this interface is that it cares about a "local" LBP in some way. That's what we're communicating back to the server... but it's worth noting that this information might be used in different ways by different load balancers. ########## core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java: ########## @@ -142,4 +151,14 @@ protected String getDriverName() { protected String getDriverVersion() { return Session.OSS_DRIVER_COORDINATES.getVersion().toString(); } + + private String localDc(DriverExecutionProfile profile) { + String dc = context.getLocalDatacenter(profile.getName()); // DC set programmatically + if (dc == null && profile.isDefined(DefaultDriverOption.LOAD_BALANCING_LOCAL_DATACENTER)) { + dc = + profile.getString( + DefaultDriverOption.LOAD_BALANCING_LOCAL_DATACENTER); // DC from configuration + } + return dc; Review Comment: This should be resolved by the LocalDcAwareLoadBalancingPolicy interface. Note that the use of that policy doesn't say anything about _how_ a given LBP uses local DC information (or even how the LBP defines the term "local"). Use of that interface is only a marker indicating that the LBP cares about a "local" DC in some way. ########## core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java: ########## @@ -142,4 +150,29 @@ protected String getDriverName() { protected String getDriverVersion() { return Session.OSS_DRIVER_COORDINATES.getVersion().toString(); } + + private String localDcs() { + StringBuilder result = new StringBuilder(); + boolean first = true; + for (Map.Entry<String, LoadBalancingPolicy> entry : + context.getLoadBalancingPolicies().entrySet()) { + String dc = getLocalDc(entry.getValue()); + if (dc != null) { + if (!first) { + result.append(", "); + } else { + first = false; + } + result.append(entry.getKey()).append(": ").append(dc); + } + } + return first ? null : result.toString(); + } + + private String getLocalDc(LoadBalancingPolicy loadBalancingPolicy) { Review Comment: Not essential by any means, but probably a good place to replace null with an Optional return type ########## core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java: ########## @@ -142,4 +150,29 @@ protected String getDriverName() { protected String getDriverVersion() { return Session.OSS_DRIVER_COORDINATES.getVersion().toString(); } + + private String localDcs() { + StringBuilder result = new StringBuilder(); + boolean first = true; + for (Map.Entry<String, LoadBalancingPolicy> entry : + context.getLoadBalancingPolicies().entrySet()) { + String dc = getLocalDc(entry.getValue()); + if (dc != null) { + if (!first) { + result.append(", "); + } else { + first = false; + } + result.append(entry.getKey()).append(": ").append(dc); + } + } + return first ? null : result.toString(); Review Comment: Semi-nit: this is the kind of thing the streaming API is really good at: ```java // do not cache local DC as it can change within LBP implementation localDcs().ifPresent(s -> builder.put(DRIVER_LOCAL_DC, s)); ... private Optional<String> localDcs() { Joiner joiner = Joiner.on(": "); Map<String, Optional<String>> lbpToDc = Maps.transformValues(context.getLoadBalancingPolicies(), this::getLocalDc); if (lbpToDc.isEmpty()) return Optional.empty(); return Optional.of(lbpToDc.entrySet().stream() .filter(e -> e.getValue().isPresent()) .map(entry -> joiner.join(entry.getKey(), entry.getValue().get())) .collect(Collectors.joining(", "))); } private Optional<String> getLocalDc(LoadBalancingPolicy loadBalancingPolicy) { return (loadBalancingPolicy instanceof LocalDcAwareLoadBalancingPolicy) ? Optional.of(((LocalDcAwareLoadBalancingPolicy) loadBalancingPolicy).getLocalDatacenter()) : Optional.empty(); } ``` -- 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
