JAVA-1932: Send DRIVER_NAME and DRIVER_VERSION in Startup #1087

emerkle826 · 2018-08-24T20:07:08Z

Change SessionBuilder so that it can take a custom map of startup options to use when building a DriverContext.
Change DefaultDriverContext to build a map of startup options that appends any custom options set on the SessionBuilder to the list of Internal options sent.
Change ProtocolInitHandler to use the startup options from the DriverContext when constructing a Startup message. This requires JAVA-1932: Send DRIVER_NAME and DRIVER_VERSION in Startup datastax/native-protocol#24 to be merged.

The end result here is for the DriverContext to allow for custom startup options to be added to the map of options we are already sending in a Startup message.

olim7t · 2018-08-27T23:19:50Z

core/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.java

+  public SelfT withStartupOptions(@Nullable Map<String, String> options) {
+    this.startupOptions = options;
+    return self;
+  }


Maybe we should call this with[Additional|Extra|Custom]StartupOptions to emphasize the fact that CQL version and compression are not required (in fact for compression it is probably even recommended to not mess with it manually, and instead let the driver inspect the configured compressor).

This method also needs javadocs to explain all this.

Nit: could we also move it after withClassLoader? I know that ultimately method order does not matter, but it's convenient to have all the boilerplate withXxx together, and then the more interesting methods.

olim7t · 2018-08-27T23:30:39Z

core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContext.java

+    NullAllowingImmutableMap.Builder<String, String> builder = NullAllowingImmutableMap.builder();
+    builder.putAll(new InternalStartupOptions(this).getOptions());
+    if (options != null) {
+      builder.putAll(options);


NAIM.builder() does not support duplicate entries. Won't that be a problem if the client passes CQL_VERSION or COMPRESSION in their map?
We should either prevent that, or check the map to allow the user to override those fields (although the use case is dubious).

olim7t · 2018-08-27T23:44:20Z

core/src/main/java/com/datastax/oss/driver/internal/core/context/InternalStartupOptions.java

+import net.jcip.annotations.Immutable;
+
+@Immutable
+public class InternalStartupOptions {


👍 to use a separate utility class, this makes tests easier. But:

I find the name a bit misleading, because in the rest of the code "startup options" refers to a Map<String, String>. This shows in DefaultDriverContext, where we have a buildStartupOptions method that constructs an InternalStartupOptions, but eventually returns a map.

currently the caller still has to merge the additional options manually, why not make this part of the utility class as well?

I'm thinking of turning it into a builder:

Map<String, String> options = new StartupOptionsBuilder(context) .withAdditionalOptions(additionalOptions) .build();

build() would contain the logic of the current buildOptions(), while also avoiding duplicates if they were already provided in additionalOptions.

olim7t · 2018-08-27T23:45:23Z

core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContext.java

+   * Builds a map of options to use when sending a Startup message. The options argument passed in
+   * will append to, or overwrite, the Internal default options sent by the driver.
+   */
+  protected Map<String, String> buildStartupOptions(Map<String, String> options) {


👍 to make this protected.

olim7t · 2018-08-27T23:50:29Z

core/src/test/java/com/datastax/oss/driver/internal/core/channel/ProtocolInitHandlerTest.java

-    writeInboundFrame(readOutboundFrame(), TestResponses.clusterNameResponse("someClusterName"));
-    assertThat(connectFuture).isSuccess();
-  }
-


👍 that makes sense, the only thing this was testing was the options map, and since it's now built outside of the handler, covering it here wouldn't be very useful.

olim7t · 2018-08-27T23:53:21Z

core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContextTest.java

+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+
+public class DefaultDriverContextTest {


This should be called InternalStartupOptionsTest (or the corresponding test name if we rename that class).

olim7t · 2018-08-28T00:00:19Z

core/src/test/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContextTest.java

+  }
+
+  @Test
+  public void test_default_startup_options() {


Nit: could you use the should_ convention for consistency with the rest of the codebase? For example:
should_build_minimal_startup_options
should_build_startup_options_with_compression
should_build_startup_options_with_custom_options

And I would also cover the cases where the custom options contain one of the default ones or the compression.

Yes, I forgot about this from my last PR. I need to break my old habit of test method names.

olim7t · 2018-08-31T00:45:26Z

core/src/main/java/com/datastax/oss/driver/internal/core/context/DefaultDriverContext.java

+  @NonNull
+  @Override
+  public Map<String, String> getStartupOptions() {
+    return startupOptionsBuilderRef.get().build();


The options are now rebuilt on every call. Did you want to leave the door open in case we ever need an option that would change for every new connection?

No, that wasn't the intent. I imagine the options won't change over the course of normal operation.

olim7t · 2018-08-31T00:47:02Z

core/src/main/java/com/datastax/oss/driver/internal/core/context/StartupOptionsBuilder.java

+   * constructor will add {@link
+   * com.datastax.oss.protocol.internal.request.Startup#CQL_VERSION_KEY}.
+   *
+   * <p>Additional options can be set via {@link #withAdditionalOptions(java.util.Map)}.


This is now outdated.
withAdditionalOptions is referenced in two other comments.

Good catch!

olim7t suggested changes Aug 28, 2018

View reviewed changes

emerkle826 force-pushed the java1932 branch 3 times, most recently from 3b804c5 to 34db712 Compare August 29, 2018 20:19

olim7t approved these changes Aug 31, 2018

View reviewed changes

JAVA-1932: Send DRIVER_NAME and DRIVER_VERSION in Startup message

ec93b8d

olim7t force-pushed the java1932 branch from 82560eb to ec93b8d Compare September 4, 2018 04:36

olim7t merged commit 791656a into 4.x Sep 4, 2018

olim7t deleted the java1932 branch September 4, 2018 15:59

absurdfarce mentioned this pull request Nov 5, 2025

CASSJAVA-116: Retry or Speculative Execution with RequestIdGenerator throws "Duplicate Key" #2060

Open

JAVA-1932: Send DRIVER_NAME and DRIVER_VERSION in Startup #1087

JAVA-1932: Send DRIVER_NAME and DRIVER_VERSION in Startup #1087

Uh oh!

Conversation

emerkle826 commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants