2

I am working with Cassandra and using Datastax Java driver. I am trying to reuse prepared statements by caching it.

  private static final Map<String, PreparedStatement> holder = new ConcurrentHashMap<>();

  public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps = holder.get(cql);
    // no statement is cached, create one and cache it now.
    if (ps == null) {
      synchronized (this) {
        ps = holder.get(cql);
        if (ps == null) {
          ps = session.prepare(cql);
          holder.put(cql, ps);
        }
      }
    }
    return ps.bind();
  }

My above getStatement method will be called by multiple threads so I have to make sure it is thread safe. I am working with Java 7 so cannot use computeIfAbsent unfortunately.

When I ran my code against static anaylysis tool, it gave me this minor warning which makes me to think is there any better way to write above code in Java 7?

Might be better to replace use of get/check/put with putIfAbsent

Update:

  public BoundStatement getStatement(String cql) {
    Session session = TestUtils.getInstance().getSession();
    PreparedStatement ps = holder.get(cql);
    // no statement is cached, create one and cache it now.
    if (ps == null) {
      ps = session.prepare(cql);
      PreparedStatement old = holder.putIfAbsent(cql, ps);
      if (old!=null)
        ps=old;
    }
    return ps.bind();
  }
john
  • 11,311
  • 40
  • 131
  • 251
  • 2
    This is wrong for a number of reasons. For one thing, you're guarding a static field by synchronizing on an instance. For another, you're mixing synchronization and lock-free collections. – shmosel Jan 01 '17 at 19:19
  • Your approach assumes that it is worth caching the prepared statement instances; is it? What is the actual performance hit of constructing a new instance each time? (Genuine question; I rarely deal with them, so have no idea how heavyweight they are). – Andy Turner Jan 01 '17 at 19:23
  • 1
    If I don't cache these prepared statements, then all my logs are filled up with warning message `Re-preparing already prepared query . Please note that preparing the same query more than once is generally an anti-pattern and will likely affect performance. Consider preparing the statement only once.` from datastax java driver. Here is the [question](http://stackoverflow.com/questions/22915840/re-using-preparedstatement-when-using-datastax-cassandra-driver) which talks more on that so that is why I decided to reuse prepared statements. – john Jan 01 '17 at 19:25

1 Answers1

1

It's not too bad the way you have it, except that one thread can block another even if they're not trying to make the same prepared statement.

Using computeIfAbsent in Java 8 would indeed be much better. In Java 7, you can do this:

ps = holder.get(cql);
if (ps == null) {
  ps = session.prepare(cql);
  PreparedStatement old = holder.putIfAbsent(cql, ps);
  if (old!=null)
    ps=old;
}

You will occasionally make an unnecessary PreparedStatement if two threads try to make the same one at the same time, but in the worst case that's just equivalent to not using a cache.

Alternatively, if you can use the guava libraries, then the guava LoadingCache does exactly what you want: https://google.github.io/guava/releases/16.0/api/docs/com/google/common/cache/CacheBuilder.html

Matt Timmermans
  • 53,709
  • 3
  • 46
  • 87