CASSJAVA-92: Local DC provided for nodetool clientstats

[lukasz-antoniak]

Fixes: CASSJAVA-92

[absurdfarce]

An implementation change I'd like to see... and I do think we need to understand what we're trying to accomplish with this feature. I'm going to start a conversation on the JIRA ticket to make sure everybody's on the same page w.r.t. goals and meaning.

[absurdfarce]

DefaultDriverContext already defines lazy instantiation 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.

[lukasz-antoniak]

I have noticed that and though that other entries of the STARTUP options are added here. 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.

[absurdfarce]

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; 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.

[absurdfarce]

config here is just the default profile 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.

[absurdfarce]

Raised

[absurdfarce]

I believe all my concerns have been addressed by changes in this PR and discussion on CASSJAVA-92.

[absurdfarce]

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.

[absurdfarce]

Not essential by any means, but probably a good place to replace null with an Optional return type

[absurdfarce]

Semi-nit: this is the kind of thing the streaming API is really good at:

    // 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();
  }

[lukasz-antoniak]

I applied your changes and also added on more unit test to make sure that non-local aware LBPs are skipped.

[absurdfarce]

@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.