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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
