3
\$\begingroup\$

My Java program is a module called configProxy which fetches configuration entries from a remote node (say from a Redis instance). When the caller calls the get(configKey) method, this module checks in a local cache (a concurrent hashmap) to see if the setting is already cached. If not, it fetches the setting from a Redis instance, caches it and then returns.

The implementation of this singleton class:

public class ConfigProxy {

/** This is our one-n-only instance **/
private static ConfigProxy _instance = null;

/** default values for some keys which may not be declared **/
private HashMap<String, String> _defaults = null;

private Jedis jedisConfigConnection;

// hash map for storing the data of config proxy as a cache for each
// String,List and Hash
// to prevent constant hitting to Redis
private ConcurrentHashMap<String, String> _cacheStrings = new ConcurrentHashMap<String, String>();
private ConcurrentHashMap<String, List<String>> _cacheLists = new ConcurrentHashMap<String, List<String>>();
private ConcurrentHashMap<String, HashMap<String, Object>> _cacheHashes = new ConcurrentHashMap<String, HashMap<String, Object>>();


/*
 * Takes host and port as parameters to connect with redis instance
 */
private ConfigProxy(String host, int port) throws ConnectionException {

    try {

        jedisConfigConnection = new Jedis(host, port);
        jedisConfigConnection.ping();
    } catch (Exception e) {
        throw new ConnectionException("Unable to connect to Config Server");
    }

}

public static ConfigProxy getInstance(String host, int port)
        throws ConnectionException {

    if (_instance == null) _instance = new ConfigProxy(host, port);
    return _instance;
}

/**
 * Overloaded version which doesn't take any parameters. Inorder for this
 * call to succeed, the caller, somewhere should have called
 * getInstance(host, port), once. else this throws InitializationException.
 * 
 * @return
 * @throws NotInitializedException
 */
public static ConfigProxy getInstance() throws NotInitializedException {
    if (_instance == null) throw new NotInitializedException();
    return _instance;
}

...

/**
 * Returns a configuration setting value either from cache or from a
 * configured Redis instance.
 * 
 *  If the given setting was not cached, and it needs to be fetched
 *  from Redis, then the code locks this object 
 * @param key
 * @return The value configured for the specified key, if one exists.
 */
public String get(String key) {
    if (_cacheStrings.containsKey(key))
        return _cacheStrings.get(key);
    else {
        synchronized (jedisConfigConnection) {
            String res = (String) jedisConfigConnection.get(key);
            if (res == null) return res;
            _cacheStrings.put(key, res);
            return res;
        }
    }
}

...
...
}

Currently, if the key already exists in the cache, the get(configkey) doesn't lock, and simply returns the value from cache. However the get function (shown at the bottom in the above code), obtains a lock on jedis connection object, before attempting to fetch data from Redis.

I want to know:

  • If this is correct, or if I obtain a lock on the entire "this" (configProxy) object.

  • If it is possible to improve it in any way to further reduce locking contentions, as this will be used in a multi-threaded high txn oriented environment.

EDIT

After review, this is my next version of the get method.

public String get(String key) {

    if (_cacheStrings.containsKey(key)) return _cacheStrings.get(key);
    else {
        String res = null;
        synchronized (jedisConfigConnection) { res = (String) jedisConfigConnection.get(key); }
        if (res != null) _cacheStrings.putIfAbsent(key, res);
        return res;
    }
}
\$\endgroup\$
2
\$\begingroup\$
  1. Definitely do not lock on the ConfigProxy instance. Somebody else could acquire a lock on it and then you'd be up the creek. Only lock on private instances that you have the only copy of.

  2. Strictly speaking, you could use putIfAbsent and get rid of the lock altogether, but then you're potentially seeing multiple Redis calls for the same key. You should check both outside and inside the synchronized block to see if the key is there yet. If two threads both hit that barrier in the else, the first one will put a value in the map .. then the second one will go do it again. The first check keeps you out of the synchronized block, and the second one lets you exit it early.

  3. I'm not entirely convinced that a Concurrent map buys you anything here, since you're managing the puts using synchronization. I might be wrong - there may be black magic around gets that I'm unaware of.

  4. Random convention stuff - Java doesn't use underscores to indicate instance variables. Always use curly braces. Make everything final that you can.

\$\endgroup\$
  • \$\begingroup\$ Hi Eric, Trying to reduce sync code, by only putting jedis call in sync block as below: public String get(String key) { if (_cacheStrings.containsKey(key)) return _cacheStrings.get(key); else { String res = null; synchronized (jedisConfigConnection) { res = (String) jedisConfigConnection.get(key); } if (res != null) _cacheStrings.put(key, res); return res; } Hope this helps..? \$\endgroup\$ – Mopparthy Ravindranath Mar 1 '15 at 7:31
  • \$\begingroup\$ I shd have realized that "add comment" doesn't format the text so we can't paste code here :( \$\endgroup\$ – Mopparthy Ravindranath Mar 1 '15 at 7:31
  • \$\begingroup\$ @MupparthyRavindranath You should look at the accepted answer to this question for the correct way to use putIfAbsent: http://stackoverflow.com/questions/3752194/best-practice-to-use-concurrentmaps-putifabsent \$\endgroup\$ – Eric Stein Mar 1 '15 at 19:56

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.