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

Support CSC(client-side caching) on broadcasting mode #4057

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wp973
Copy link

@wp973 wp973 commented Jan 16, 2025

Hi, I added support for broadcast mode tracking based on the current csc implementation.

Could you please help me see if this is possible?

Thanks~

Design Schematic:
image

code example:

    @Test
    public void testTrackingOnBroadcastMode() {
        // tracking prefix list(only broadcast mode)
        List<String> trackingPrefixList = new ArrayList<>();
        trackingPrefixList.add("v1");
        trackingPrefixList.add("v2");

        JedisClientConfig clientConfig = DefaultJedisClientConfig.builder()
                .resp3()                                      // RESP3 protocol is required for client-side caching
                .trackingModeOnDefault(false)                 // tracking mode(true:default; false:broadcast)
                .trackingPrefixList(trackingPrefixList)       // tracking prefix list(only broadcast mode)
                .build();

        HostAndPort node = HostAndPort.from("127.0.0.1:6379");

        try (UnifiedJedis client = new UnifiedJedis(node, clientConfig, CacheFactory.getCache(cacheConfig))) {
            String a = client.get("a");
            System.out.println();
        }
    }

pu.wang added 4 commits January 15, 2025 20:54
…t_mode

# Conflicts:
#	src/main/java/redis/clients/jedis/DefaultJedisClientConfig.java
#	src/main/java/redis/clients/jedis/JedisPooled.java
@wp973 wp973 marked this pull request as draft January 17, 2025 03:29
@wp973 wp973 marked this pull request as ready for review January 17, 2025 07:57
@wp973 wp973 marked this pull request as draft January 17, 2025 07:58
@wp973
Copy link
Author

wp973 commented Jan 17, 2025

Hi~ @sazzad16, could you please help me see if this is possible when you have time?

@sazzad16
Copy link
Collaborator

@wp973 Thank you for your consideration and effort in improving Jedis.

Firstly and importantly, if I am not mistaken, this PR implements the "Two connections mode" mentioned here. At this moment, we are not considering that approach to include in Jedis.

Secondly, the implementation in this PR does not seem to easily expandable to cluster mode (JedisCluster). This is one of our key considerations.

So, we may not consider this PR for the time being. Please don't be discouraged. We hope you will continue to help/suggest improving Jedis.

@wp973
Copy link
Author

wp973 commented Jan 21, 2025

@sazzad16 Thank you very much for your reply.

Firstly and importantly, if I am not mistaken, this PR implements the "Two connections mode" mentioned here. At this moment, we are not considering that approach to include in Jedis.

This PR implements the broadcast mode, not the two connections mode. The command used is:

CLIENT TRACKING ON BCAST PREFIX xxx

@sazzad16
Copy link
Collaborator

@wp973 Got it.

However, whatever I said before are still mostly applicable.

So please don't consider following as my review but here are a few hints:

  1. Connection.connect() is not helpful. It is there as a legacy method. You should look for other options.
    For example, creating a new (Cache)Connection is better than connect().
  2. If you are using the same JedisClientConfig as data connections to create CacheConnection for tracking, getTrackingModeOnDefault=false actually makes the tracking connection not to track.
    You may look for other options to receive new config params instead of JedisClientConfig.
  3. How can you shutdown the ScheduledExecutorService?
  4. (Not right now, may be at a later stage) How to expand the idea to cluster mode?

@wp973 wp973 closed this Jan 21, 2025
@wp973
Copy link
Author

wp973 commented Jan 21, 2025

However, whatever I said before are still mostly applicable.

So please don't consider following as my review but here are a few hints:

  1. Connection.connect() is not helpful. It is there as a legacy method. You should look for other options.
    For example, creating a new (Cache)Connection is better than connect().
  2. If you are using the same JedisClientConfig as data connections to create CacheConnection for tracking, getTrackingModeOnDefault=false actually makes the tracking connection not to track.
    You may look for other options to receive new config params instead of JedisClientConfig.
  3. How can you shutdown the ScheduledExecutorService?
  4. (Not right now, may be at a later stage) How to expand the idea to cluster mode?

@sazzad16 This is very helpful for me, thanks! I will continue to improve this pr.

@wp973 wp973 reopened this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants