Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding CacheConfig #3919

Merged
merged 11 commits into from
Aug 28, 2024
Merged

Adding CacheConfig #3919

merged 11 commits into from
Aug 28, 2024

Conversation

atakavci
Copy link
Contributor

adding CacheConfig and CacheProvider

@atakavci atakavci requested review from sazzad16, uglide and tishun August 14, 2024 11:18
Copy link

@tishun tishun left a comment

Choose a reason for hiding this comment

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

I assume we decided to enable the user to provide the implementation of the Cache so that they could have the option to provide a custom one. Do we want to have a constructor that defaults to the DefaultCache?

Another thing I am wondering is - we could provide a custom implementation for the cache (based on the interface) perhaps we need to be able to provide custom eviction policies ?

@sazzad16 sazzad16 marked this pull request as draft August 25, 2024 15:15
@atakavci atakavci marked this pull request as ready for review August 26, 2024 14:40
@atakavci atakavci requested a review from tishun August 26, 2024 14:40
@atakavci
Copy link
Contributor Author

I assume we decided to enable the user to provide the implementation of the Cache so that they could have the option to provide a custom one. Do we want to have a constructor that defaults to the DefaultCache?

@tishun, i believe this one is resolved as of now. let me know if it not.

Another thing I am wondering is - we could provide a custom implementation for the cache (based on the interface) perhaps we need to be able to provide custom eviction policies ?

added evictionpolicy to builder

Copy link
Contributor

@uglide uglide left a comment

Choose a reason for hiding this comment

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

LGTM. Only one missing constructor overload in UnifiedJedis

src/main/java/redis/clients/jedis/UnifiedJedis.java Outdated Show resolved Hide resolved
-  wrap IOException on protocol read error
uglide
uglide previously approved these changes Aug 28, 2024
Copy link
Contributor

@uglide uglide left a comment

Choose a reason for hiding this comment

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

👍

tishun
tishun previously approved these changes Aug 28, 2024
@atakavci atakavci dismissed stale reviews from tishun and uglide via aa5f27f August 28, 2024 14:38
@atakavci atakavci merged commit 88cf9bc into redis:5.2.0 Aug 28, 2024
3 checks passed
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

In all of Jedis repo, if a constructor forwards to another one, the caller is placed before/above the callee. Could you please maintain that? (I can do it after all other requests are resolved.)

Comment on lines +238 to 240
this(new ClusterConnectionProvider(clusterNodes, clientConfig, clientSideCache), maxAttempts,
maxTotalRetriesDuration,
clientConfig.getRedisProtocol(), clientSideCache);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this(new ClusterConnectionProvider(clusterNodes, clientConfig, clientSideCache), maxAttempts,
maxTotalRetriesDuration,
clientConfig.getRedisProtocol(), clientSideCache);
this(new ClusterConnectionProvider(clusterNodes, clientConfig, clientSideCache), maxAttempts,
maxTotalRetriesDuration, clientConfig.getRedisProtocol(), clientSideCache);

} catch (IOException e) {
// TODO handle it properly
throw new JedisConnectionException("Failed to read pending buffer for push messages !", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
throw new JedisConnectionException("Failed to read pending buffer for push messages !", e);
throw new JedisConnectionException("Failed to read pending buffer for push messages!", e);

}

public static class Builder {
private int maxSize;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Set the default value.


import java.util.HashMap;

public class CacheProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should rename this class. IMHO, the way the term Provider is used in this repo doesn't go along with the usage/behavior of this class.

(Suggestions: CacheManager, CacheUtil, CacheConfigManager, CacheConfigHandler.)

Comment on lines +7 to +14
public Cache getCache(CacheConfig config) {
return getCache(config, new HashMap<CacheKey, CacheEntry>());
}

public Cache getCache(CacheConfig config, HashMap<CacheKey, CacheEntry> map) {
return new DefaultCache(config.getMaxSize(), map, config.getCacheable(),
getEvictionPolicy(config));
}
Copy link
Collaborator

@sazzad16 sazzad16 Aug 28, 2024

Choose a reason for hiding this comment

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

Just asking/discussing: How about renaming getCache to createCache?

BTW, if this class is rename to CacheManager or CacheUtil, createCache or simply create makes more sense.

@atakavci
Copy link
Contributor Author

hey @sazzad16, i just saw your comments after merge, let me address each in new PR

sazzad16 added a commit that referenced this pull request Aug 28, 2024
Polish #3919 - address some pending change requests

- Swap contructor placements
- Fix grammar in exception message
uglide added a commit that referenced this pull request Sep 27, 2024
* Initial support for client-side caching (#3658)

* Support for client-side caching - phase 2 (#3673)

* Code re-use?

* Stop forcing to read push notifications before checking cache and remove BCAST

* Rename variable

* Remove ensureFillSafe()

* Refactor peeking and reading push notifications

* Cleanup comments

* Fix transaction failure tests using mock (#3683)

Now we have to mock Protocol#read(RedisInputStream, ClientSideCache) instead of Protocol#read(RedisInputStream).

* Support client-side caching from UnifiedJedis (#3691)

* Support client side caching from UnifiedJedis

* Support client side caching as a separate parameter

* format imports

* Support CSC in sentinel mode

* undo change

* Client-side caching by hashing command arguments (#3700)

* Support TTL in client side caching (using Caffeine library)

* Also Guava cache

* format pom.xml

* Client-side caching by command arguments

TODO: Compute hash code.

* send keys

* todo comment for clean-up

* rename method to invalidate

* Client-side caching by hashing command arguments

* Hash command arguments for CaffeineCSC using OpenHFT hashing

* Clean-up keyHashes map

* added javadoc

* rename method

* remove lock

* descriptive name

* descriptive names and fix

* common default values in base class

* Cover Redis commands for client side caching (#3702)

* Support Client-side caching through URI/URL (#3703)

* Support Client-side caching through URI/URL

* check idx of '=' sign

* nicer exception

* edit/fix condition

* rename param

* Throw IllegalArgumentException at all such cases

* Test GuavaCSC and CaffeineCSC (#3742)

* Support white-list and black-list commands and keys (#3755)

* Create csc package

* Create csc.util package

* Create a config interface for client-side caching

* Default isCacheable

* Config to WhiteList/BlackList commands and String keys

* Create csc test package(s)

* Test white-list/black-list commands and keys

* Merge fix

* Remove csc.util package

* Fix javadoc links

* Added ClientSideCacheable interface and removed ClientSideCacheConfig interface

* Format imports

* Re-create csc.util package

* Rename to allow/deny instead of white/black

* Introduce interface(s) for hashing CommandObject (#3743)

* Client-side cache related naming changes (#3758)

Changes:
1. CommandLongHashing is renamed to CommandLongHasher.
2. Expanded the names of GuavaCSC (GuavaClientSideCache) and CaffeineCSC (CaffeineClientSideCache).

* Reformat clientSideCache variable names (#3761)

* Format tabs in pom.xml

* Use Experimental annotation

* Fix client side cache tests (#3799)

Due to redis/redis#13167 

* Fix JedisClusterClientSideCacheTest

* Fix JedisSentineledClientSideCacheTest

* Remove openhft hashing from source dependency (#3800)

* Test different functionalities of client side cache (#3828)

* Test JedisURIHelper#getClientSideCache(URI) (#3835)

* Merge fix: after introducing EndpointConfig in #3836

* Tweak maximumSize test in CaffeineClientSideCacheTest

* Little more tweak maximumSize test in CaffeineClientSideCacheTest

* Fix incompatibilities with the latest RedisStack (#3855)

* Fix tests

- Skip Graph tests
- Fix JSON RESP3 test

* JSON.GET behaves identically on RESP2 and RESP3

* Revert "Fix incompatibilities with the latest RedisStack (#3855)"

This reverts commit 6b9d338.

* [TEMPORARY] [TEST] Use redis-stack-server:7.4.0-rc1 image for testing

* Support RediSearch DIALECT 5 (#3831)

- [x] Avoid escaping at query time
- [ ] Alias for tag fields (EXACT)
- [x] Avoid repeating for numeral equality
- [x] New dialect (5)

* Support FLOAT16 and BFLOAT16 VecSim storage types (#3849)

* Test: INTERSECTS and DIJOINT conditions support in GeoSearch (#3862)

* Support IGNORE and other optional arguments for timeseries commands (#3860)

* Re-implement TS.ADD command with optional arguments
* Implement TS.INCRBY and TS.DECRBY commands with optional arguments
* Support IGNORE argument for TS.[ CREATE | ALTER | ADD | INCRBY | DECRBY] commands

---

* Cover optional arguments for timeseries commands
   - Re-implement TS.ADD command with optional arguments
   - Implement TS.INCRBY and TS.DECRBY commands with optional arguments

* Introduce EncodingFormat enum for <COMPRESSED|UNCOMPRESSED>

* Support IGNORE option
   and rename to TSIncrOrDecrByParams

* Polish #3860: Separate params for TS.INCRBY and TS.DECRBY (#3863)

* Support indexing of MISSING and EMPTY values (#3866)

* Little tweak maximumSize test in CaffeineClientSideCacheTest

* Inject ClientSideCacheable via set method (#3882)

* Use CommandObject(s) as cache-key (#3875)

and remove hashing of CommandObject(s).

* #3886 merge fix

* Revert "[TEMPORARY] [TEST] Use redis-stack-server:7.4.0-rc1 image for testing"

This reverts commit 92c09f3.

* More tweak maximumSize test in CaffeineClientSideCacheTest

This reverts and modifies commit 3534996.

* Remove client side cache support through uri/url (#3892)

This partially reverts #3703 and #3835

* Bump com.google.guava:guava from 33.0.0-jre to 33.2.1-jre (#3893)

* Prepare client side caching - design 2 (#3889)

* Separate CacheConnection

* Introduce CacheKey and CacheEntry

* Little tweak maximumSize test in CaffeineClientSideCacheTest

* Remove resetting timeout; we'll PING instead

* Refactor Client-Side Caching implementation (#3900)

* adding a DataProvider to access connection from cache

* resolve keys from commandarguments

* clean up in unifiiedjedis and add csc test with ssl

* - fix readtimeout exception with sockets for consuming invalidations pending in buffer
- apply a default list of cacheable commands to DefaultClientSideCacheable
- fix failing unit tests with cacheable / non-cacheable keys
- remove formatting changes

* - add serialization for cache instances
- add unit test with UnifiedJedis
- add benchmark for CSC execution
- clean unused imports

* - added 'Cache' interface and 'DefaultCache' implementation in regard to design doc
- added 'EvictionPolicy' interface and LRU implementation
- move cache object validation and cache control stuf from 'ClientSideCache' into 'CacheConnection'
- make guava and caffeine caches experimental

* - added SSLSocketWrapper and plug it to use 'available'
- handle exceptions properly
- fix some issues with unit tests

* implementing thread safety

* - fix eviction issue and add related test
- fix consuming invalidation messages on a response read
- introduce cachestats
- fix potential issue with cacheKeysRelatedtoRedisKey cleanup
- tests for sequential access, concurrent acces and maxsize

* - renmae abstract cache class
- add test case for returning new instance of cache object

* - change order of execution in sequential acces test

* -  flush the cache on any disconnect
- replace LRU policy references with EvictionPolicy interface
-  add some constructor overloads to enable custom eviction policies on cache

* fix testcache

* fix javadoc issue

* - fix multithreaded eviction policy issue
- update guava and caffeine implementations according to abstract cache

* Jedis test plan coverage for CSC (#3918)

* initial changes

* cover tests for JedisPooled and functionality

* fix javadoc

* cover new tests for JedisCluster and JedisSentineled

* Fix CSC allow-and-deny-list and rename Cacheable interface

* Tag CommandArguments#getKeys() as Internal

* cover lruEvictionTest

* Address code reviews and more updates

* fix format and more minor changes

* format Connection

* modify WeakReference usage

* Use ExecutorService.shutdownNow() in tests (#3922)

* Use ExecutorService.shutdownNow()

* More ExecutorService.shutdownNow() and other changes

* [minor change] Avoid creating same CacheKey twice

* Support caching null values (#3939)

* caching null results

* add more assertion

* Adding CacheConfig (#3919)

* add cacheconfig

* remove empty file

* -modify constructors with cache as public
- trim guava caffeine

* remove cachetype

* - add getCache to UnifiedJedis
- add builder method to CacheConfig

* add evictionpolicy to cacheconfig

* - unifiedjedis constructor with cacheconfig
-  wrap IOException on protocol read error

* fix merge issue

---------

Co-authored-by: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com>

* Polish "Adding CacheConfig"

Polish #3919 - address some pending change requests

- Swap contructor placements
- Fix grammar in exception message

* Adding Cache class to CacheConfig (#3942)

* adding cacheclass to cacheconfig

* - add cachefactory test

* - revert connection ctors to public
- udpate some tests with UnifiedJedis.getCache
- add ping to flaky tests

* remove unnecessary anonymous types

* change ctor access modifiers

* fix test name

* make cachefactory methods static

* removing pings  due to still flaky with inv messages

* - drop CustomCache in tests and use TestCache
- check null cacheable issue with defaultcache
- support both ctors in custom cache classes regarding to value of cacheconfig.cacheable

* remove unncessary maxsize

* - remove inline anonymious

* Server version check for CSC activation  (#3954)

* checking server version for CSC

* fix format change

* fix noauth hello exception in integration tests

* fix version check

* remove redundant check

* remove unused imports

* 'toString' for Version

* rename to RedisVersion

* moving RedisVersion package

---------

Co-authored-by: Igor Malinovskiy <u.glide@gmail.com>
Co-authored-by: atakavci <58048133+atakavci@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants