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

Rename __redis__:invalidate to __valkey__:invalidate from client-side-caching #410

Closed
wants to merge 2 commits into from

Conversation

Virusuki
Copy link
Contributor

@Virusuki Virusuki commented May 1, 2024

Change the name of the internal tracking channel handled by client side caching from redis to valkey.

And then, Update in the corresponding tracking.c, tracking.tcl, and client-eviction.tcl files.

I tested Client tracking through the valkey server.

valkey_server
client7_8_9

The test environment has three clients and one server.

  • The ID of the first client is 7
  • The ID of the second client is 8
  • The ID of the third client is 9

Test scenario:
I checked the ID value of each client.

Firstly, in Client 7(ID), command "Client tracking on redirect 8".
Enter the subscribe valkey:invalidate command on Client 8 (ID), and it is in the subscribe state.
Check the current value with get Tracking_key command on Client 7 (ID).
Result: (nill)

Next, enter the “set Tracking_key value1” command on Client 9 (ID).
At the same time, confirm that a message has been received from Client 8 (ID).

  1. "message"
  2. "valkey:invalidate"
  3. 1 "Tracking_key"
    You can check the changed Tracking_key value in Client 7 (ID) as picture of client 7.

Lastly, When Client 9(ID) changes back to value5 of Tracking_key, You can see that the valkey:invalidate invalidation message was received from client 8 (ID).
It can check the changed Tracking_key value again in Client 7 (ID).

In conclusion, it was confirmed through testing that it was processed without problems.

…-caching

Signed-off-by: NAM UK KIM <namuk2004@naver.com>
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.46%. Comparing base (4948d53) to head (8796bfb).
Report is 16 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #410      +/-   ##
============================================
+ Coverage     68.37%   68.46%   +0.08%     
============================================
  Files           108      109       +1     
  Lines         61562    61671     +109     
============================================
+ Hits          42096    42220     +124     
+ Misses        19466    19451      -15     
Files Coverage Δ
src/tracking.c 98.99% <100.00%> (ø)

... and 18 files with indirect coverage changes

Signed-off-by: NAM UK KIM <namuk2004@naver.com>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

This is the issue: #280.

It explains what we want.

@Virusuki
Copy link
Contributor Author

Virusuki commented May 2, 2024

This is the issue: #280.

It explains what we want.

I realize the situation that simply changing redis... -> valkey... is not compatible with Redis OSS 7.2.
I confirmed that various ideas were discussed.
Thank you for letting me know, and sorry for any confusion.

In my opinion, it taking risks and making changes will have side effects.

I agree with what you said below.
@zuiderkwast As you said,

My suggestion is that we check if the client is subscribed to valkey:invalidate and if yes, we send it on that channel. Otherwise, we send it on redis:invalidate to keep backwards compatibility. (This is for RESP2 only btw.)

Afterwards, I think it would be a good idea to support value:invalidate in Valkey version 8.

Thank you for your feedback.

@Virusuki Virusuki closed this May 5, 2024
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.

None yet

2 participants