absurdfarce commented on code in PR #2036:
URL:
https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2221104112
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -142,4 +152,28 @@ protected String getDriverName() {
protected String getDriverVersion() {
return Session.OSS_DRIVER_COORDINATES.getVersion().toString();
}
+
+ private Optional<String> driverBaggage() {
+ Joiner joiner = Joiner.on(": ");
+ Map<String, Optional<String>> lbpToBag =
+ Maps.transformValues(context.getLoadBalancingPolicies(),
this::getDriverBaggage);
+ return Optional.of(
+ "{"
+ + lbpToBag.entrySet().stream()
+ .filter(e -> e.getValue().isPresent())
+ .map(
+ entry ->
+ joiner.join(Strings.doubleQuote(entry.getKey()),
entry.getValue().get()))
+ .collect(Collectors.joining(", "))
+ + "}");
Review Comment:
I completely agree with @aratno on this one. I'd actually go a bit further;
I'd argue it's the responsibility of individual LBPs to return a collection of
name/value pairs as a Map with absolutely no notion of JSON formatting. It's
then the responsibility of StartupOptionsBuilder (or any tool that wants to
format these values in any way, either as JSON or something else) to put them
into the appropriate format for their use case.
As a proof-of-concept the following implementation passes the test you added
to DseStartupOptionsBuilderTest @lukasz-antoniak . I'm not saying this has to
be the impl... I'm just providing it as a concrete example of what I'm talking
about (and I _think_ what @aratno is talking about as well, although I don't
want to speak for him):
```diff
diff --git
a/core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java
b/core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java
index d5604b4bf..55ed157e6 100644
---
a/core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java
+++
b/core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java
@@ -20,6 +20,8 @@ package com.datastax.oss.driver.api.core.loadbalancing;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
+import java.util.Map;
+
/** Load balancing policy taking into account local datacenter of the
application. */
public interface LocalDcAwareLoadBalancingPolicy extends
LoadBalancingPolicy {
@@ -29,5 +31,5 @@ public interface LocalDcAwareLoadBalancingPolicy extends
LoadBalancingPolicy {
/** Returns JSON string containing all properties that impact C* node
connectivity. */
@NonNull
- String getStartupConfiguration();
+ Map<String, ?> getStartupConfiguration();
}
diff --git
a/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java
b/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java
index f64472761..55412fcf5 100644
---
a/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java
+++
b/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java
@@ -23,16 +23,21 @@ import
com.datastax.oss.driver.api.core.loadbalancing.LoadBalancingPolicy;
import
com.datastax.oss.driver.api.core.loadbalancing.LocalDcAwareLoadBalancingPolicy;
import com.datastax.oss.driver.api.core.session.Session;
import com.datastax.oss.driver.api.core.uuid.Uuids;
+import
com.datastax.oss.driver.internal.core.type.codec.extras.json.JsonCodec;
import com.datastax.oss.driver.internal.core.util.Strings;
import com.datastax.oss.driver.shaded.guava.common.base.Joiner;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
import com.datastax.oss.driver.shaded.guava.common.collect.Maps;
import com.datastax.oss.protocol.internal.request.Startup;
import
com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap;
+import com.fasterxml.jackson.databind.ObjectMapper;
import edu.umd.cs.findbugs.annotations.Nullable;
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
import net.jcip.annotations.Immutable;
@Immutable
@@ -154,21 +159,24 @@ public class StartupOptionsBuilder {
}
private Optional<String> driverBaggage() {
- Joiner joiner = Joiner.on(": ");
- Map<String, Optional<String>> lbpToBag =
- Maps.transformValues(context.getLoadBalancingPolicies(),
this::getDriverBaggage);
- return Optional.of(
- "{"
- + lbpToBag.entrySet().stream()
- .filter(e -> e.getValue().isPresent())
- .map(
- entry ->
- joiner.join(Strings.doubleQuote(entry.getKey()),
entry.getValue().get()))
- .collect(Collectors.joining(", "))
- + "}");
+ ImmutableMap.Builder builder = new ImmutableMap.Builder();
+ for (Map.Entry<String,LoadBalancingPolicy> entry :
context.getLoadBalancingPolicies().entrySet()) {
+ this.getDriverBaggage(entry.getValue()).ifPresent(baggage -> {
+ builder.put(entry.getKey(), baggage);
+ });
+ }
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+
+ return Optional.of(mapper.writeValueAsString(builder.build()));
+ }
+ catch (Exception e) {
+ e.printStackTrace();
+ return Optional.empty();
+ }
}
- private Optional<String> getDriverBaggage(LoadBalancingPolicy
loadBalancingPolicy) {
+ private Optional<Map<String,?>> getDriverBaggage(LoadBalancingPolicy
loadBalancingPolicy) {
if (loadBalancingPolicy instanceof LocalDcAwareLoadBalancingPolicy) {
LocalDcAwareLoadBalancingPolicy dcAwareLbp =
(LocalDcAwareLoadBalancingPolicy) loadBalancingPolicy;
diff --git
a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java
b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java
index 777fa66ce..e62c724db 100644
---
a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java
+++
b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java
@@ -46,9 +46,12 @@ import
com.datastax.oss.driver.internal.core.util.collection.CompositeQueryPlan;
import com.datastax.oss.driver.internal.core.util.collection.LazyQueryPlan;
import com.datastax.oss.driver.internal.core.util.collection.QueryPlan;
import
com.datastax.oss.driver.internal.core.util.collection.SimpleQueryPlan;
+import com.datastax.oss.driver.shaded.guava.common.base.Joiner;
import com.datastax.oss.driver.shaded.guava.common.base.Predicates;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
import com.datastax.oss.driver.shaded.guava.common.collect.Lists;
import com.datastax.oss.driver.shaded.guava.common.collect.Sets;
+import com.fasterxml.jackson.databind.ObjectMapper;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import java.nio.ByteBuffer;
@@ -65,6 +68,7 @@ import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.IntUnaryOperator;
import java.util.stream.Collectors;
import net.jcip.annotations.ThreadSafe;
+import org.apache.tinkerpop.shaded.kryo.util.ObjectMap;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -165,43 +169,20 @@ public class BasicLoadBalancingPolicy implements
LocalDcAwareLoadBalancingPolicy
@NonNull
@Override
- public String getStartupConfiguration() {
- StringBuilder builder = new StringBuilder();
- builder
- .append("{")
-
.append(Strings.doubleQuote(BasicLoadBalancingPolicy.class.getSimpleName()))
- .append(":")
- .append("{")
- .append(Strings.doubleQuote("localDc"))
- .append(":")
- .append(Strings.doubleQuoteNullable(localDc));
+ public Map<String, ?> getStartupConfiguration() {
+
+ ImmutableMap.Builder builder = new ImmutableMap.Builder<>();
+ builder.put("localDc", localDc);
if (!preferredRemoteDcs.isEmpty()) {
- builder
- .append(",")
- .append(Strings.doubleQuote("preferredRemoteDcs"))
- .append(":[")
- .append(
- preferredRemoteDcs.stream()
- .map(Strings::doubleQuote)
- .collect(Collectors.joining(", ")))
- .append("]");
+ builder.put("preferredRemoteDcs", preferredRemoteDcs);
}
if (allowDcFailoverForLocalCl) {
- builder
- .append(",")
- .append(Strings.doubleQuote("allowDcFailoverForLocalCl"))
- .append(":")
- .append(allowDcFailoverForLocalCl);
+ builder.put("allowDcFailoverForLocalCl", allowDcFailoverForLocalCl);
}
if (maxNodesPerRemoteDc > 0) {
- builder
- .append(",")
- .append(Strings.doubleQuote("maxNodesPerRemoteDc"))
- .append(":")
- .append(maxNodesPerRemoteDc);
+ builder.put("maxNodesPerRemoteDc", maxNodesPerRemoteDc);
}
- builder.append("}}");
- return builder.toString();
+ return ImmutableMap.of(BasicLoadBalancingPolicy.class.getSimpleName(),
builder.build());
}
/** @return The nodes currently considered as live. */
diff --git
a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
index 533239a6d..146b4b41c 100644
---
a/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
+++
b/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java
@@ -34,6 +34,7 @@ import
com.datastax.oss.driver.internal.core.util.ArrayUtils;
import com.datastax.oss.driver.internal.core.util.collection.QueryPlan;
import
com.datastax.oss.driver.internal.core.util.collection.SimpleQueryPlan;
import
com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
import com.datastax.oss.driver.shaded.guava.common.collect.MapMaker;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.Nullable;
@@ -353,12 +354,8 @@ public class DefaultLoadBalancingPolicy extends
BasicLoadBalancingPolicy impleme
@NonNull
@Override
- public String getStartupConfiguration() {
- String result = super.getStartupConfiguration();
- result =
- result.replaceFirst(
- BasicLoadBalancingPolicy.class.getSimpleName(),
- DefaultLoadBalancingPolicy.class.getSimpleName());
- return result;
+ public Map<String, ?> getStartupConfiguration() {
+ Map<String, ?> parent = super.getStartupConfiguration();
+ return
ImmutableMap.of(DefaultLoadBalancingPolicy.class.getSimpleName(),
parent.get(BasicLoadBalancingPolicy.class.getSimpleName()));
}
}
diff --git
a/core/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.java
b/core/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.java
index 15e4bcbfc..99175265b 100644
---
a/core/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.java
+++
b/core/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.java
@@ -339,9 +339,9 @@ public class DseStartupOptionsBuilderTest {
assertThat(startup.options)
.containsEntry(
StartupOptionsBuilder.DRIVER_BAGGAGE,
- "{\"oltp\": {\"DefaultLoadBalancingPolicy\":{"
+ "{\"oltp\":{\"DefaultLoadBalancingPolicy\":{"
+ "\"localDc\":\"dc1\","
- + "\"preferredRemoteDcs\":[\"dc2\", \"dc3\"],"
+ + "\"preferredRemoteDcs\":[\"dc2\",\"dc3\"],"
+ "\"allowDcFailoverForLocalCl\":true,"
+ "\"maxNodesPerRemoteDc\":2}}}");
}
```
There are some changes to the test code itself but they're only minor
whitespace changes; the substance is still very much intact. Regardless this
demonstrates the key aspect of the change; LBPs provide metadata as Maps and
StartupOptionsBuilder handles the JSON conversion by passing everything off to
Jackson.
--
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
