You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "lukasz-antoniak (via GitHub)" <gi...@apache.org> on 2025/04/14 14:42:10 UTC
[PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
lukasz-antoniak opened a new pull request, #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036
Fixes: [CASSJAVA-92](https://issues.apache.org/jira/browse/CASSJAVA-92)
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3219661796
After delegating the logic of providing startup configuration to a implementations of `LoadBalancingPolicy`, I think we do not need `LocalDcAwareLoadBalancingPolicy` any more. This simplifies the changes and addresses some concerns pointed by @absurdfarce above.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3363812947
Jenkins run looks good. If @lukasz-antoniak is good with the most recent changes I think we're (finally!) all done with this one.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3196138825
I am not sure why all tests passed when I was running tests locally. Apologies for that.
Indeed test `AllNodesFailedIT` shows chicken and egg problem with discovering local datacenter. As far as I understand the intention form the server side, driver should publish local DC value that was passed in application configuration. I have slightly changed `OptionalLocalDcHelper` and extracted a method to return configured local data center if any was specified.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2305404957
##########
core/src/test/java/com/datastax/dse/driver/internal/core/context/DseStartupOptionsBuilderTest.java:
##########
@@ -185,12 +276,72 @@ public void should_use_configuration_when_no_programmatic_values_provided() {
public void should_ignore_configuration_when_programmatic_values_provided() {
when(defaultProfile.getString(DefaultDriverOption.PROTOCOL_COMPRESSION, "none"))
.thenReturn("none");
+ when(defaultProfile.getName()).thenReturn(DriverExecutionProfile.DEFAULT_NAME);
- buildContext(null, "Custom_App_Name", "Custom_App_Version");
+ buildContext(
+ null, "Custom_App_Name", "Custom_App_Version", ImmutableMap.of("default", "us-west-2"));
Startup startup = new Startup(driverContext.getStartupOptions());
assertThat(startup.options)
.containsEntry(StartupOptionsBuilder.APPLICATION_NAME_KEY, "Custom_App_Name")
- .containsEntry(StartupOptionsBuilder.APPLICATION_VERSION_KEY, "Custom_App_Version");
+ .containsEntry(StartupOptionsBuilder.APPLICATION_VERSION_KEY, "Custom_App_Version")
+ .containsEntry(
+ StartupOptionsBuilder.DRIVER_BAGGAGE,
+ "{\"default\":{\"DefaultLoadBalancingPolicy\":{\"localDc\":\"us-west-2\"}}}");
+ }
+
+ @Test
+ public void should_include_all_local_dc_in_startup_message() {
+ when(defaultProfile.getString(DefaultDriverOption.PROTOCOL_COMPRESSION, "none"))
+ .thenReturn("none");
+ when(defaultProfile.getName()).thenReturn(DriverExecutionProfile.DEFAULT_NAME);
+
+ buildContext(
+ null,
+ "Custom_App_Name",
+ "Custom_App_Version",
+ ImmutableMap.of("default", "us-west-2", "oltp", "us-east-2", "olap", "eu-central-1"));
+ Startup startup = new Startup(driverContext.getStartupOptions());
+
+ assertThat(startup.options)
+ .containsEntry(StartupOptionsBuilder.APPLICATION_NAME_KEY, "Custom_App_Name")
+ .containsEntry(StartupOptionsBuilder.APPLICATION_VERSION_KEY, "Custom_App_Version")
+ .containsEntry(
+ StartupOptionsBuilder.DRIVER_BAGGAGE,
+ "{\"default\":{\"DefaultLoadBalancingPolicy\":{\"localDc\":\"us-west-2\"}},"
+ + "\"oltp\":{\"DefaultLoadBalancingPolicy\":{\"localDc\":\"us-east-2\"}},"
+ + "\"olap\":{\"DefaultLoadBalancingPolicy\":{\"localDc\":\"eu-central-1\"}}}");
+ }
+
+ @Test
+ public void should_include_all_lbp_details_in_startup_message() {
+ when(defaultProfile.getString(DefaultDriverOption.PROTOCOL_COMPRESSION, "none"))
+ .thenReturn("none");
+ when(defaultProfile.getName()).thenReturn(DriverExecutionProfile.DEFAULT_NAME);
+
+ ProgrammaticArguments.Builder builder = ProgrammaticArguments.builder();
+ DefaultDriverContext driverContext =
+ new DefaultDriverContext(configLoader, builder.build()) {
+ @NonNull
+ @Override
+ public Map<String, LoadBalancingPolicy> getLoadBalancingPolicies() {
+ ImmutableMap.Builder<String, LoadBalancingPolicy> map = ImmutableMap.builder();
+ map.put(
+ "oltp",
+ mockLoadBalancingPolicy("oltp", "dc1", 2, true, ImmutableList.of("dc2", "dc3")));
+ map.put("default", mock(LoadBalancingPolicy.class));
+ return map.build();
+ }
+ };
+ Startup startup = new Startup(driverContext.getStartupOptions());
+
+ assertThat(startup.options)
+ .containsEntry(
+ StartupOptionsBuilder.DRIVER_BAGGAGE,
+ "{\"oltp\":{\"DefaultLoadBalancingPolicy\":{"
+ + "\"localDc\":\"dc1\","
+ + "\"preferredRemoteDcs\":[\"dc2\",\"dc3\"],"
+ + "\"allowDcFailoverForLocalCl\":true,"
+ + "\"maxNodesPerRemoteDc\":2}}}");
}
}
Review Comment:
Semi-nit: there's nothing DSE-specific about this test that I can see. Shouldn't this just live in [StartupOptionsBuilderTest](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/test/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilderTest.java)?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3242613892
I have implemented re-evaluation of startup options upon every request and renamed test class. I am not quite satisfied with disabling option. It was hard to come up with a decent name for this configuration parameter (see last commit). I think that the payload of driver baggage will be so small in size, that maybe we should leave it enabled all the time? @aratno, are you OK with leaving driver baggage enabled and provide this feature when time comes and this payload will grow?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2305400342
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -119,6 +129,8 @@ public Map<String, String> build() {
if (applicationVersion != null) {
builder.put(APPLICATION_VERSION_KEY, applicationVersion);
}
+ // do not cache local DC as it can change within LBP implementation
Review Comment:
Maybe I'm missing something but... isn't this _exactly_ what we're doing here? We're in the middle of StartupOptionsBuilder.build() which is only called when DefaultDriverContext is building a new StartupOptions instance... and it only does that once (lazily, the first time it's needed). So if information becomes available later... how is it ever updated here?
Bad thing is the creation of Startup instances [only takes](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandler.java#L178) whatever the context gives you for startup options... so there isn't a whole lot of room to work with here. You could probably fix this by making DefaultDriverContext.getStartupOptions() do something beyond just returning the static list of startup options... maybe you could query the LBPs there and update any information (if updates are to be had).
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2220036816
##########
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:
Why not depend on Jackson to do the serialization?
I was thinking you could add a DriverBaggage class with `String toJson()` and `static DriverBaggage put(String key, DriverBaggage value)`, and in StartupOptionsBuilder here add all the load balancing policies' baggage. Then we wouldn't have the implicit "String that's actually JSON" type.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java:
##########
@@ -155,10 +158,52 @@ public BasicLoadBalancingPolicy(@NonNull DriverContext context, @NonNull String
* Before initialization, this method always returns null.
*/
@Nullable
- protected String getLocalDatacenter() {
+ @Override
+ public String getLocalDatacenter() {
return localDc;
}
+ @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));
Review Comment:
Same comment as above about using Jackson for JSON encoding
##########
core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LocalDcAwareLoadBalancingPolicy.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.NonNull;
+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();
+
+ /** Returns JSON string containing all properties that impact C* node connectivity. */
+ @NonNull
+ String getStartupConfiguration();
Review Comment:
See comment below about String vs DriverBaggage. Could hoist this up to LoadBalancingPolicy with an empty default as well.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2043754945
##########
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:
DefaultDriverContext already defines [lazy instantiation](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContext.java#L337-L348) for (and access to) the startup options map for a given run. Rather than splitting the logic for determining the contents of a STARTUP message between DefaultDriverContext and this class the majority of the logic in this class should be consolidated into the existing DefaultDriverContext methods.
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -119,6 +122,12 @@ public Map<String, String> build() {
if (applicationVersion != null) {
builder.put(APPLICATION_VERSION_KEY, applicationVersion);
}
+ if (applicationLocalDc == null) {
+ applicationLocalDc = localDc(config);
Review Comment:
_config_ here is [just the default profile](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java#L95) but any profile can define a local datacenter... which in turn means we need to think about how execution profiles interact with this functionality.
I'm going to raise this point on the JIRA ticket; I think it warrants further discussion.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2307640418
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -142,4 +154,21 @@ protected String getDriverName() {
protected String getDriverVersion() {
return Session.OSS_DRIVER_COORDINATES.getVersion().toString();
}
+
+ private Optional<String> driverBaggage() {
+ ImmutableMap.Builder<String, Object> builder = new ImmutableMap.Builder<>();
+ for (Map.Entry<String, LoadBalancingPolicy> entry :
+ context.getLoadBalancingPolicies().entrySet()) {
+ Map<String, ?> config = entry.getValue().getStartupConfiguration();
Review Comment:
We should offer a configuration to disable getStartupConfiguration that matches the current behavior, for users with tons of profiles or custom policies that don't support it properly yet
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak merged PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3363608370
I've pushed changes to remove the config option to disable baggage collection per the latest review from @aratno. @lukasz-antoniak I'd like one last review of [those changes](https://github.com/apache/cassandra-java-driver/pull/2036/commits/9bba5ea586a129644131696c7c3c9c8a213549e8) from you before we call this done.
I'm also gonna kick off one last Jenkins run, just to make sure we didn't have any unexpected regressions here.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2303107158
##########
core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LoadBalancingPolicy.java:
##########
@@ -76,6 +76,12 @@ default Optional<RequestTracker> getRequestTracker() {
*/
void init(@NonNull Map<UUID, Node> nodes, @NonNull DistanceReporter distanceReporter);
+ /** Returns map containing details that impact C* node connectivity. */
+ @NonNull
+ default Optional<Map<String, ?>> getStartupConfiguration() {
Review Comment:
I changed the code to use a `Map`. For now I did not create new class, because once we use it in different places, maybe the lifecycle will be different. Let us change it once need comes.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-2936743235
@aratno If you have some cycles do you mind taking a look at this and giving me a second 👍 ? I know you and @smiklosovic are still in conversation about the server-side of this but I think we're in a good spot to at least call the client-side done.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2048233664
##########
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:
Mentioned to @lukasz-antoniak in separate conversation but I'll document is here as well; checking for the presence of this config prop isn't enough. You also need to see whether you have an LBP implementation which cares about that prop; it's entirely possible a user could define LOAD_BALANCING_LOCAL_DATACENTER and then use some other LBP which just ignores it.
It's also worth noting that some LBPs can infer the local DC if it isn't present... which means they may not have anything defined for this property but would still have a notion of a local DC.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3359205735
Note: [CASSJAVA-112](https://issues.apache.org/jira/browse/CASSJAVA-112) is the follow-up ticket [discussed above](https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3293837576).
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2307678409
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -119,6 +129,8 @@ public Map<String, String> build() {
if (applicationVersion != null) {
builder.put(APPLICATION_VERSION_KEY, applicationVersion);
}
+ // do not cache local DC as it can change within LBP implementation
Review Comment:
Agree - hadn't noticed this before, sounds like we'd need to make DefaultDriverContext#startupOptions() not a LazyReference, so each call is refreshed rather than set once on first access.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2048227807
##########
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:
Okay, as I look at this again you're right, there's something of a bifurcation here already. The entries returned by DefaultDriverContext.buildStartupOptions() are more static key/value pairs, mostly (exclusively?) pairs that were used by Insights. Nearly all of those should be removed as part of [CASSJAVA-73](https://issues.apache.org/jira/browse/CASSJAVA-73); driver name and version will stay but the rest should disappear.
So how _should_ we format this data? That question is still under discussion in CASSJAVA-92... we probably need to settle on what the data should look like and then adjust this impl accordingly.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2043882546
##########
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:
I have noticed that and though that other entries of the STARTUP options are added [here](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java#L99-L103). The justification of the logic would be that all "dedicated" properties for STARTUP message are lazily instantiated where you pointed out, whereas all properties taken from other components (e.g. compression) are automatically injected in `build()` method.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2399393724
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -142,4 +154,21 @@ protected String getDriverName() {
protected String getDriverVersion() {
return Session.OSS_DRIVER_COORDINATES.getVersion().toString();
}
+
+ private Optional<String> driverBaggage() {
+ ImmutableMap.Builder<String, Object> builder = new ImmutableMap.Builder<>();
+ for (Map.Entry<String, LoadBalancingPolicy> entry :
+ context.getLoadBalancingPolicies().entrySet()) {
+ Map<String, ?> config = entry.getValue().getStartupConfiguration();
Review Comment:
Yeah, fine with me
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3215800631
Thanks for the fixes @lukasz-antoniak. I can confirm that these fixes address the ConnectionInitExceptions I mentioned in my prior message.
There's still something that has been gnawing at me about this impl but despite a hefty amount of effort I haven't been able to cleanly articulate my concern. I _think_ at the core it's the idea that we implement an extra interface (LocalDcAwareLoadBalancingPolicy) here to indicate that an LBP is aware of it's local DC, a change that in general I completely agree with. But then we say that this functionality can only be determined in _some_ cases (i.e. if a user configures a DC in some way). This covers the default case but that's only due to the implementation of DefaultLoadBalancingPolicy. We have an extension of the default policy named [DcInferringLoadBalancingPolicy](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DcInferringLoadBalancingPolicy.java) which will try to "infer" the local DC (based on contact points) if nothing is specified. This LBP may or may not have a value... again, it d
epends entirely on the impl. It's a similar story for custom LBP impls.
A reasonable question to ask here is "if we change LocalDcAwareLoadBalancingPolicy.getLocalDatacenter() to return an Optional<String> rather than a String doesn't that address the problem?" Maybe it does... and I think that might be a very reasonable change. But it's not _completely_ satisfying; we're still saying we have this whole interface because an LBP is aware of it's local DC... until it's not.
I'm still not explaining this very well, and for that I apologize.
@aratno does _any_ of this resonate for you or am I just nuts?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2302370075
##########
core/src/main/java/com/datastax/oss/driver/api/core/loadbalancing/LoadBalancingPolicy.java:
##########
@@ -76,6 +76,12 @@ default Optional<RequestTracker> getRequestTracker() {
*/
void init(@NonNull Map<UUID, Node> nodes, @NonNull DistanceReporter distanceReporter);
+ /** Returns map containing details that impact C* node connectivity. */
+ @NonNull
+ default Optional<Map<String, ?>> getStartupConfiguration() {
Review Comment:
Why not make this a Map and default to empty map, instead of using Optional?
Could also make DriverBaggage its own class, since it's used in a few different places and we expect it to be used even more than LoadBalancingPolicies.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2043832885
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -119,6 +122,12 @@ public Map<String, String> build() {
if (applicationVersion != null) {
builder.put(APPLICATION_VERSION_KEY, applicationVersion);
}
+ if (applicationLocalDc == null) {
+ applicationLocalDc = localDc(config);
Review Comment:
[Raised](https://issues.apache.org/jira/browse/CASSJAVA-92?focusedCommentId=17944601&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17944601)
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3192790679
So, I think this addresses the serialization changes referenced above. But we are seeing a host of [IT failures](https://ci-cassandra.apache.org/job/cassandra-java-driver/job/PR-2036/8/testReport/junit/com.datastax.oss.driver.core/AllNodesFailedIT/Matrix___Matrix___TEST_JAVA_VERSION____openjdk_1_8_0_292___SERVER_VERSION____4_0____Tests___should_report_multiple_errors_per_node/) with errors similar to the following:
```
Expected size: 2 but was: 1 in:
[com.datastax.oss.driver.api.core.connection.ConnectionInitException: [s10|control|id: 0xc1585e83, L:/127.0.0.1:33080 - R:/127.0.0.14:9043] Protocol initialization request, step 2 (OPTIONS): unexpected exception (java.lang.NullPointerException: null value in entry: localDc=null)
at com.datastax.oss.driver.internal.core.channel.ProtocolInitHandler$InitRequest.fail(ProtocolInitHandler.java:358)
at com.datastax.oss.driver.internal.core.channel.ProtocolInitHandler$InitRequest.onResponse(ProtocolInitHandler.java:351)
at com.datastax.oss.driver.internal.core.channel.ChannelHandlerRequest.onResponse(ChannelHandlerRequest.java:96)
...(22 remaining lines not displayed - this can be changed with Assertions.setMaxStackTraceElementsDisplayed)]
```
Underlying problem here seems to be a variation on something we discussed earlier; some startup options aren't available at context creation time (when StartupOptionsBuilder is created). Let's focus specifically on local DC; it's computed [based on the nodes in the cluster](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java#L188-L213) and is computed [when the LBP is initialized](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/BasicLoadBalancingPolicy.java#L170). But that doesn't happen [until we create the connection pool](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/session/DefaultSession.java#L472-L481) which makes sense (given that we don't have full info about available nodes until much beyond that point).
Upshot is I don't think we can handle local DC in StartupOptionsBuilder... it's gonna have to be computed elsewhere and added to the STARTUP message at generation time.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2349973744
##########
core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java:
##########
@@ -142,4 +154,21 @@ protected String getDriverName() {
protected String getDriverVersion() {
return Session.OSS_DRIVER_COORDINATES.getVersion().toString();
}
+
+ private Optional<String> driverBaggage() {
+ ImmutableMap.Builder<String, Object> builder = new ImmutableMap.Builder<>();
+ for (Map.Entry<String, LoadBalancingPolicy> entry :
+ context.getLoadBalancingPolicies().entrySet()) {
+ Map<String, ?> config = entry.getValue().getStartupConfiguration();
Review Comment:
I don't know that I'm super-worried about this honestly. Users with custom LBPs should be okay; this PR includes an update to the primary interface com.datastax.oss.driver.api.core.loadbalancing.LoadBalancingPolicy with a default impl of getStartupConfiguration() that returns an empty map. So even if the user is working with a custom LBP that was implemented before this change they'll still be covered by the default method... and since DriverContext.getLoadBalancingPolicies() guarantees we'll get a map of Strings onto LoadBalancingPolicy impls that should cover everybody, right?
Users with lots of LBPs may have some slight overhead but it's far from clear to me that it's enough to worry about disabling the functionality all together.
Whaddya think @aratno?
##########
core/src/test/java/com/datastax/dse/driver/internal/core/context/StartupOptionsBuilderTest.java:
##########
@@ -19,6 +19,7 @@
Review Comment:
I don't think we have this quite right yet.
This change is correct in recognizing that all functionality tested here (both compression in startup message as well as the extraction of LBP information) is not specific to DSE and is supported in Cassandra proper. So renaming the functionality here to StartupOptionsBuilderTest is correct. However this class is still in the com.datastax.dse.driver.internal.core.context package, which seems inconsistent (and incorrect). This class should be moved to the com.datastax.oss.driver.internal.core.context package.
Note that we [already have](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/test/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilderTest.java) a com.datastax.oss.driver.internal.core.context.StartupOptionsBuilderTest so I think the right answer is to add this test logic to the existing class.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "absurdfarce (via GitHub)" <gi...@apache.org>.
absurdfarce commented on PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#issuecomment-3293837576
While I'm thinking of it: we should probably create a ticket for the follow-up work referenced [here](https://github.com/apache/cassandra-java-driver/pull/2036#pullrequestreview-3161907434) so that it's at least in the backlog.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2352493193
##########
core/src/test/java/com/datastax/dse/driver/internal/core/context/StartupOptionsBuilderTest.java:
##########
@@ -19,6 +19,7 @@
Review Comment:
I have moved the tests to OSS `StartupOptionsBuilderTest` and reverted all changes from DSE one.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSJAVA-92: Local DC provided for nodetool clientstats [cassandra-java-driver]
Posted by "lukasz-antoniak (via GitHub)" <gi...@apache.org>.
lukasz-antoniak commented on code in PR #2036:
URL: https://github.com/apache/cassandra-java-driver/pull/2036#discussion_r2126158693
##########
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:
I applied your changes and also added on more unit test to make sure that non-local aware LBPs are skipped.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org