1
\$\begingroup\$

I am working with Cassandra and using the Datastax Java driver for it. Here is my singleton class where it makes a connection to Cassandra:

public class CassUtil {
  private static final Logger LOGGER = Logger.getInstance(CassUtil.class);

  private Session session;
  private Cluster cluster;

  private static class Holder {
    private static final CassUtil INSTANCE = new CassUtil();
  }

  public static CassUtil getInstance() {
    return Holder.INSTANCE;
  }

  private CassUtil() {
    List<String> servers = TestUtils.HOSTNAMES;
    String username =
        TestUtils.loadCredentialFile().getProperty(TestUtils.USERNAME);
    String password =
        TestUtils.loadCredentialFile().getProperty(TestUtils.PASSWORD);

    PoolingOptions opts = new PoolingOptions();
    opts.setCoreConnectionsPerHost(HostDistance.LOCAL,
        opts.getCoreConnectionsPerHost(HostDistance.LOCAL));

    Builder builder = Cluster.builder();
    cluster =
        builder
            .addContactPoints(servers.toArray(new String[servers.size()]))
            .withRetryPolicy(DowngradingConsistencyRetryPolicy.INSTANCE)
            .withPoolingOptions(opts)
            .withReconnectionPolicy(new ConstantReconnectionPolicy(100L))
            .withLoadBalancingPolicy(
                DCAwareRoundRobinPolicy
                    .builder()
                    .withLocalDc(
                        !TestUtils.isProduction() ? "DC2" : TestUtils.getCurrentLocation()
                            .get().name().toLowerCase()).withUsedHostsPerRemoteDc(3).build())
            .withCredentials(username, password).build();

    try {
      session = cluster.connect("testkeyspace");
      StringBuilder sb = new StringBuilder();
      Set<Host> allHosts = cluster.getMetadata().getAllHosts();
      for (Host host : allHosts) {
        sb.append("[");
        sb.append(host.getDatacenter());
        sb.append(host.getRack());
        sb.append(host.getAddress());
        sb.append("]");
      }
      LOGGER.logInfo("CONNECTED SUCCESSFULLY TO CASSANDRA CLUSTER: " + sb.toString());
    } catch (NoHostAvailableException ex) {
      LOGGER.logError("error= ", ExceptionUtils.getStackTrace(ex));
    } catch (Exception ex) {
      LOGGER.logError("error= " + ExceptionUtils.getStackTrace(ex));
    }
  }


  public void shutdown() {
    LOGGER.logInfo("Shutting down the whole cassandra cluster");
    if (null != session) {
      session.close();
    }
    if (null != cluster) {
      cluster.close();
    }
  }

  public Session getSession() {
    if (session == null) {
      throw new IllegalStateException("No connection initialized");
    }
    return session;
  }

  public Cluster getCluster() {
    return cluster;
  }
}

I wanted to see if my CassUtil constructor can be improved in any way. It looks like CassUtil has 40+ lines of code. I think it can be broken down to a few methods but I am not able to figure out how I should do it.

\$\endgroup\$
  • \$\begingroup\$ Do your code compile ? Where are processMetadata, topicMetadata and procMetadata declared ? \$\endgroup\$ – Spotted Nov 30 '16 at 7:06
  • \$\begingroup\$ Sorry that was by mistake. I have edited it. I forgot to remove that part. Sorry about that. \$\endgroup\$ – david Nov 30 '16 at 17:37
1
\$\begingroup\$

First thoughts

Before even commenting the code, I have to say that the intent of this class is for me totally obscure. When I encounter some xxxUtil class, I expect to find some static utility methods. However here, it isn't the case.

Second, I'm not sure that I understand why you make this class a singleton. Yes, everytime I see a singleton I challenge that because it is very likely that it is an anti-pattern. So I will ignore the singleton for the rest of my answer.

Purpose of this class

The next thing that bothers me is that I'm not able to find the purpose of this class. It has no behaviour (except maybe shutdown()) ! All you do is creating two objects (session and cluster) and returning them. So the internal state of this class has totally leaked out (via getSession() and getCluster()) and all the work done with these two object is done outside of CassUtil, which is a significant smell in OOP world.

More concretely

Leaving these "philosophical" aspects above, let's dig a bit in something more concrete, the code itself.

Immutability

Making things immutable always help to get a more maintainable code, therefore I would advice to make CassUtil final so that noone can break it by subclassing. I would also make session and cluster final so that it becomes clear for anyone reading your code that once you instanciated them, they won't change anymore.

public final class CassUtil {
    private final Session session;
    private final Cluster cluster;

PoolingOptions

Does these two lines really do something useful other than assigning an existing value to itself ?

PoolingOptions opts = new PoolingOptions();
opts.setCoreConnectionsPerHost(HostDistance.LOCAL,opts.getCoreConnectionsPerHost(HostDistance.LOCAL));

If not, you can just remove these lines and do

.withPoolingOptions(new PoolingOptions())

StringBuilder

At the end of the constructor, you iterate through all hosts and append some content in a StringBuilder just to add it in a log statement. Wouldn't it be enough to do something like that ? (or equivalent)

LOGGER.logInfo("CONNECTED SUCCESSFULLY TO CASSANDRA CLUSTER: " + cluster.getClusterName());

Thus removing the need for the whole block constructing the StringBuilder:

StringBuilder sb = new StringBuilder();
Set<Host> allHosts = cluster.getMetadata().getAllHosts();
for (Host host : allHosts) {
    sb.append("[");
    sb.append(host.getDatacenter());
    sb.append(host.getRack());
    sb.append(host.getAddress());
    sb.append("]");
}

Get rid of TestUtils

I notice that your code depends a lot on TestUtils, which is static. And static means that your class is really hard to test. Let me reiterate: static dependencies make your class really (if not impossible) hard to test.

TestUtils provides 4 things:

  1. Hostnames
  2. A username
  3. A password
  4. Indirectly, a localDc

You better take all of these as arguments in the constructor, so that you don't need to reference TestUtils directly.

public CassUtil(List<String> servers, String username, String password, String localDc) {
    ...
}

This line

!TestUtils.isProduction() ? "DC2" : TestUtils.getCurrentLocation().get().name().toLowerCase()

is really the biggest smell of all these four because you actually mixed some production and testing concepts in the same place. That's why I advised you to take directly the localDc in argument instead of a boolean isProduction.

Creating the session

I'm not sure it makes sense here to embed the session's creation inside a try/catch. I would just remove it and let the exception bubble up, if ever. If you think it's probable exceptions will be raised from this call, then this instruction doesn't belong to the constructor, but should rather be in the method(s) where session is used (or in a connect() method, which would be the counterpart of shutdown()). So instead of

try {
    session = cluster.connect("testkeyspace");
    LOGGER.logInfo("CONNECTED SUCCESSFULLY TO CASSANDRA CLUSTER: " + cluster.getClusterName());
} catch (NoHostAvailableException ex) {
    LOGGER.logError("error= ", ExceptionUtils.getStackTrace(ex));
} catch (Exception ex) {
    LOGGER.logError("error= " + ExceptionUtils.getStackTrace(ex));
}

Just do

session = cluster.connect("testkeyspace");
LOGGER.logInfo("CONNECTED SUCCESSFULLY TO CASSANDRA CLUSTER: " + cluster.getClusterName());

Something that raised a warning in my head is the hardcoded string "testkeyspace". I suspect that this name should be different if you try to create a session in production or in testing. If so, you also need to take this string in the constructor's argument.

Remove null checks

Since now session and cluster are final, it is guaranteed that they won't ever be null. shutdown() and getSession() thus become:

public void shutdown() {
    LOGGER.logInfo("Shutting down the whole cassandra cluster");
    session.close();
    cluster.close();
}

public Session getSession() {
    return session;
}

Usage

Now that the constructor has changed (taking now 5 arguments) you might ask how to construct instances of CassUtil ?

For testing purpose:

new CassUtil(TestUtils.HOSTNAME, TestUtils.USERNAME, TestUtils.PASSWORD, "DC2", "testkeyspace");

In production:

new CassUtil(/*some real server names*/, /*username of current user*/, /*password of current user*/, /*name of the production local datacenter*/, /*production keyspace*/);

Conclusion

This is the whole class with all of these remarks:

import java.util.List;

import com.datastax.driver.core.Cluster;
import com.datastax.driver.core.PoolingOptions;
import com.datastax.driver.core.Session;
import com.datastax.driver.core.policies.ConstantReconnectionPolicy;
import com.datastax.driver.core.policies.DCAwareRoundRobinPolicy;
import com.datastax.driver.core.policies.DowngradingConsistencyRetryPolicy;

public final class CassUtil {
    private static final Logger LOGGER = Logger.getInstance(CassUtil.class);
    private final Session session;
    private final Cluster cluster;

    public CassUtil(List<String> servers, String username, String password, String localDc, String keyspace) {
        cluster = Cluster.builder().addContactPoints(servers.toArray(new String[servers.size()]))
                                   .withRetryPolicy(DowngradingConsistencyRetryPolicy.INSTANCE)
                                   .withPoolingOptions(new PoolingOptions())
                                   .withReconnectionPolicy(new ConstantReconnectionPolicy(100L))
                                   .withLoadBalancingPolicy(DCAwareRoundRobinPolicy.builder().withLocalDc(localDc)
                                                                                             .withUsedHostsPerRemoteDc(3)
                                                                                             .build())
                                   .withCredentials(username, password)
                                   .build();
        session = cluster.connect(keyspace);
        LOGGER.logInfo("CONNECTED SUCCESSFULLY TO CASSANDRA CLUSTER: " + cluster.getClusterName());
    }

    public void shutdown() {
        LOGGER.logInfo("Shutting down the whole cassandra cluster");
        session.close();
        cluster.close();
    }

    public Session getSession() {
        return session;
    }

    public Cluster getCluster() {
        return cluster;
    }
}

Now your class looks more clean but also presents the terrible signs of anemia (as expressed in the first two chapters of this answer) ! Think about that class: "which services should this class offer ?". Unfortunately, I can't answer that for you. But I'm sure as soon as you find the answer you will be able to find it a better name and add it new methods that actually contains business logic.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.