Skip to content
This repository was archived by the owner on May 30, 2024. It is now read-only.

Conversation

@ghost
Copy link

@ghost ghost commented May 16, 2016

Added:

  • Configuration options to set the Jedis pool connection and socket timeouts in the RedisFeatureStore.
  • Support for 3 modes of RedisFeatureStore local cache behaviour; default evict, refresh and async refresh (see javadoc for more details on functionality).
  • A RedisFeatureStoreConfig with it's own builder. Deprecated the old constructors for the RedisFeatureStore (I was a little daring here - deprecation isn't necessary of course).
  • Tests for the RedisFeatureStoreBuilder.

Summary of the change:

We found that having the ability to set the conn and socket timeouts in the Jedis pool was useful to provide a lower hard limit on blocking refreshes. Given the default cache behaviour is a blocking expireAfterWrite (http://google.github.io/guava/releases/snapshot/api/docs/src-html/com/google/common/cache/CacheBuilder.html#line.623) - it's useful to be able to set these parameters to something lower than the default 2 seconds.

Additionally the new modes of cache operation are situationally quite useful. Specifically utilising the asynchronous refresh "mode" which decouples network I/O from stale values fully, returning the previously cached value (FeatureRep<>) in the meantime. This should theoretically provide a optimal local cache performance, at the cost of "greater" eventual consistency.

These additions were all backed by the introduction of a RedisFeatureStoreBuilder much like the LDClient one. This simplifies the constructor the of the RedisFeatureStore and improving it's flexibility and ease of construction.

* @param port the port for the Redis connection
* @param prefix a namespace prefix for all keys stored in Redis
* @param cacheTimeSecs an optional timeout for the in-memory cache. If set to 0, no in-memory caching will be performed
* @deprecated as of X. Please use the {@link com.launchdarkly.client.RedisFeatureStore#RedisFeatureStore(RedisFeatureStoreConfig)} constructor for a more
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to set X to if we do decide deprecating is the way to go with these constructors

@ghost ghost force-pushed the ivorp/redis-lazy-refresh branch from e291b1e to dbca075 Compare May 18, 2016 00:42
@drichelson
Copy link
Contributor

Thanks for the contribution @ivorp2 !

* @param port the port for the Redis connection
* @param prefix a namespace prefix for all keys stored in Redis
* @param cacheTimeSecs an optional timeout for the in-memory cache. If set to 0, no in-memory caching will be performed
* @deprecated as of X. Please use the {@link com.launchdarkly.client.RedisFeatureStore#RedisFeatureStore(RedisFeatureStoreBuilder)} constructor for a more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a v1.1 release, so X should read 1.1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jkodumal
Copy link
Contributor

@ivorp2 one minor change to the Javadocs, and this looks ready to merge. Thanks for the thorough documentation!

 - Configuration options to set the Jedis pool connection and socket timeouts in the RedisFeatureStore.
 - Support for 3 modes of RedisFeatureStore local cache behaviour; default evict, refresh and async refresh.
 - A RedisFeatureStoreBuilder with it's own builder. Deprecated the old constructors for the RedisFeatureStore.
 - Tests for the RedisFeatureStoreBuilder.
@ghost ghost force-pushed the ivorp/redis-lazy-refresh branch from ee6b348 to 29cf485 Compare May 20, 2016 06:23
@ghost
Copy link
Author

ghost commented May 20, 2016

@jkodumal ah of course! Just updated the deprecation doco and rebased all the extra PR commits.

Hopefully it should be good to go now.

@jkodumal jkodumal merged commit 35696c7 into launchdarkly:master May 20, 2016
eli-darkly added a commit that referenced this pull request Feb 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants