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 wt option in setCommand #1109

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

Conversation

charsyam
Copy link
Contributor

1] spec

  • SET has a new option WT, it only will set the key if the new weight is > previous weight with wt option
    • if tx_time value is larger than current weight it will return +OK
    • otherwise return -ERR
    • but it can always update key's value without "wt" option.
    • we should give weight when we use wt option.
set [key] [value] wt [weight]
  • Every key has a new property called "tx" like as "expire"
    • we can get tx value using txCommand
    • if key exists but it doesn't have tx_time, it returns 0
    • if key doesn't exist, it returns -1
    • return tx_time
weight [key]

2] weight

  • we can use db operation timestamp as weight.
  • or we can get global timestamp from other server(such as redis)

3] usage example

set a 123 wt 1000000
+OK
set a 123 wt 10000 <--- it only will set the key with larger weight
-ERR
weight a
1000000
set a 123 wt 2000000
+OK
weight b
-1
set c 123
+OK
weight c
0
set a 234  <---- exception case.
+OK

4] Why this feature is needed?

  • when someone wants redis to use a cache, They will do them following these sequence.

    1. read db
    2. write redis
  • Normal Scenario.

    1. Process A - Read key A(1) from DB at 1ms
    2. Other Proceess - Write key A(2) to DB.
    3. Process B - Read key A(2) from DB at 3ms
    4. Process A - Write set A 1 to Redis at 4ms
    5. Process B - Write set A 2 to Redis at 5ms
  • Race Condition Scenario. Key(Value)

    1. Process A - Read key A(1) from DB at 1ms
    2. Other Proceess - Write key A(2) to DB.
    3. Process B - Read key A(2) from DB at 3ms
    4. Context-Switching in Process A or Sleep happens.
    5. Process B - Write set A 2 to Redis at 4ms
    6. Process A - Write set A 1 to Redis at 5ms
  • Race Condition. consistency is broken.

  • Using set wt Scenario.

    1. Process A - Read key A(1) from DB at 1ms
    2. Other Proceess - Write key A(2) to DB.
    3. Process B - Read key A(2) from DB at 3ms
    4. Context-Switching in Process A or Sleep happens.
    5. Process B - Write set A 2 wt 3 to Redis at 4ms
    6. Process A - Write set A 1 wt 1 Redis at 5ms
  • but "set A 1 wt 1" failed. it can keep consistency.

5] drawback

  1. it spends more memory to keep db->weights
  2. RDB saving and loading is changed. so previous version redis can't read new rdb.

I believe this feature is very useful when we design cache layer. Thank you.

@flutia
Copy link

flutia commented May 16, 2013

This is a cool feature what I've expected! Thank you.
And I'd like to make a suggestion.
Why don't you clarify that 'tx' can be an any number(even a negative number) or only a timestamp number? Considering its purpose, I think that it's not a fault that 'tx' can hold a negative number or a small bit that is for saving memory.

@AllenDou
Copy link

@charsyam, I like this feature, but adding this feature could cause a lot of problems.
First, since you add nx to set* command , what about sadd,zadd, etc.
Second, how to get tx(i think tx is timestamp), who get it, for example, there are two clients A, B,but machine A & machine B have inaccurate time clock. what will happen? how to solve it?
That's my personal opinion.
thx.

@badboy
Copy link
Contributor

badboy commented May 16, 2013

Could you please explain what this does? The code lacks any documentation and the commit message is useless. What does tx stand for?

@charsyam
Copy link
Contributor Author

@flutia I think it is better not to allow less than 0 value.
and 0 means it has no tx time, -1 means there is no key. Thank you :)

@AllenDou I added txCommand that can get tx time from key. and I don't consider zadd and sadd because set's other options also don't support (set or sorted set). yes. tx means timestamp and it always uses extra parameter from client.
for example,

set a 123 tx 100000 <- this is tx time

and I append txCommand. I also think it is needed.

and inaccurate time doens't matter. because tx-time for db's operation time and client send those kind of values.

@badboy
for example. There are some processes: they read db and update cache. please see the above scenario.
it can prevent those kind of scenarios. To avoid race condition in updating cache in two processes. I think this feature
will be useful.

@badboy
Copy link
Contributor

badboy commented May 16, 2013

I still don't fully get it. So the timestamp refers to the time the set is written, so that any other client SETting the same key with a timestamp before that will fail? Or is it that the key is locked until that timestamp?

@antirez
Copy link
Contributor

antirez commented May 16, 2013

Dudes, sorry but there is no description at all about what is the semantics of TX. Please write what it does :-)

Like:

  • Every key has a new hidden property called "tx".
  • SET has a new option TX, it only will set the key if the previous value is <= the hidden property.

And so forth. Not sure if the above is correct at all, just an example.

@charsyam
Copy link
Contributor Author

@flutia @AllenDou @badboy @antirez Thank you, guys. I updated this issue description. feel free to ask about this feature.

@flutia
Copy link

flutia commented May 16, 2013

@charsyam Ok, I fully agree with you. If zero means that a key has no tx time, allowing negative number can cause confusion. I think that positive numbers are enough in most cases. :)

Let me explain about this issues.
There is a common pitfall when one constructs a DB-and-set-cache design.
While a thread that have updated some values to DB are suspending (and does not set the cache with the values yet), another thread can update the DB with new values, and done to set cache with new values. As a result, DB has the new values but the cache holds the old values that the former thread set.

To avoid that situation, for example, memcached provides a 'cas' operation. The operation works well, but one should fetch(get()) a key first. So, it requires two network steps and the steps can be a unnecessary burden in simple DB-and-set-cache case.

I think that a timestamp fetched from a DB is eligible for 'tx'. However someone who want use this feature will select their own value for 'tx'. :)

@badboy
Copy link
Contributor

badboy commented May 16, 2013

Thanks, @charsyam, now the idea is clear.

@k8nx
Copy link

k8nx commented May 23, 2013

+1

@junegunn
Copy link

I think we can do this with Lua scripting as shown below though it doubles the number of keys.

local ts = redis.call('get', KEYS[2]) or -1
if tonumber(ARGV[2]) > tonumber(ts) then
  redis.call('mset', KEYS[1], ARGV[1], KEYS[2], ARGV[2])
  return ARGV[1]
else
  return {err="Stale"}
end
> evalsha 3715540b1dead19e7d3fe146e87bb5d157e41c81 2 k k:ts hello 100
"hello"
> evalsha 3715540b1dead19e7d3fe146e87bb5d157e41c81 2 k k:ts world 200
"world"
> evalsha 3715540b1dead19e7d3fe146e87bb5d157e41c81 2 k k:ts goodbye 150
(error) Stale

What do you think?

@charsyam
Copy link
Contributor Author

@junegunn I think it has some troubles. for example if you use memory policy,
at that time, k:ts can be removed. so I think it is better tx attribute to become key attribute such as expire.
and other guys can easily remove it without any intension.

@junegunn
Copy link

@charsyam Good point. But what if we just pack them into a single list?
Then we don't have to worry about separate eviction of those two.

local ts = redis.call('lindex', KEYS[1], 1) or -1
if tonumber(ARGV[2]) > tonumber(ts) then
  redis.call('del', KEYS[1])
  redis.call('rpush', KEYS[1], ARGV[1], ARGV[2])
  return ARGV[1]
else
  return {err='Stale'}
end

@charsyam
Copy link
Contributor Author

@junegunn Thank you for your interesting.
I think this way is also possible. but,
first of all, It needs more overhead for operation.
second, I don't make sure that using another structure is good way to provide this feature.
Don't make me wrong. maybe second reason is my own philosophy.
Thank you for your interesting again. :) If you can I want to talk with you in Korean. :)

@junegunn
Copy link

Ok. I think the question here is: "Is it worth it?"
Is it worth it to integrate this feature into the core, when what it tries
to achieve is already not impossible with the current set of features.
Should we make this certain pattern of usage easier by extending the core,
or should we just keep the core simple? Obviously I'm not the one to decide
that, but I'm leaning towards the latter.

(You can drop me an e-mail if you'd like, the address can be found on my
github page. Thanks.)

2013/5/23 charsyam notifications@github.com

@junegunn https://github.com/junegunn Thank you for your interesting.
I think this way is also possible. but,
first of all, It needs more overhead for operation.
second, I don't make sure that using another structure is good way to
provide this feature.
Don't make me wrong. maybe second reason is my own philosophy.
Thank you for your interesting again. :) If you can I want to talk with
you in Korean. :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1109#issuecomment-18331285
.

cheers,
junegunn choi.

@charsyam
Copy link
Contributor Author

@junegunn If you just ask my opinion? I make sure that it is worth to do.
As you said, we can do many things using scripting. But we also consider its.performance and usescases. When I designed cache layer this feature is important. And facebook or other big company uses this feature. So it should be simple and fast. Scripting is.slower and using collections spend much memeory and call more function. So it is.better this feature is merged into core.
Yes, core should be simple. And i.think this.feature doesn,t break simpleness of core.

@charsyam
Copy link
Contributor Author

@antirez, @flutia, @AllenDou, @badboy, @dgkim84, @junegunn
I tested this feature through comparing native and lua scripting.

totally, I tested each case for 4 times. and it tested 100,000 times per test

set 100,000 keys
--speed(second)(native wins)
     | native | lua
1   | 15.809| 23.130 
2   | 15.737| 23.165
3   | 15.544| 23.819
4   | 15.590| 23.724

--memory(lua wins: using other containers)
     | native | lua
1   | 12.02M| 11.17M 
2   | 12.02M| 11.17M 
3   | 12.02M| 11.17M 
4   | 12.02M| 11.17M 

I used this test generating script: https://gist.github.com/charsyam/5638416

To my surprise, Using other containers uses less memories than dictEntry as set-wt attribute.(78%)
but set-tx native approach is faster than lua-scripting(46
53%)

and I increased test case to test 1,000,000 times per test

set 1,000,000 keys
--speed(second)(set wt native wins)
     | native | lua
1   | 2m37  | 3m59 
2   | 2m39  | 3m58

--memory(set wt native wins)
     | native | lua
1   | 108M  | 207M 
2   | 108M  | 207M 

Yes, set wt is 50% faster and uses 50% memories compare to lua scripting. so I think set-wt native implementation is better than using lua scripting. Thank you.

feel free to point out my mistakes :) Thank you.

@AllenDou
Copy link

good!

set-tx

change name tx to weight
@charsyam
Copy link
Contributor Author

I changed tx to wt and weight. because it is more meaningful compare to before.

yossigo added a commit to yossigo/redis that referenced this pull request May 30, 2023
b6a052fe0 Helper for setting TCP_USER_TIMEOUT socket option (redis#1188)
3fa9b6944 Add RedisModule adapter (redis#1182)
d13c091e9 Fix wincrypt symbols conflict
5d84c8cfd Add a test ensuring we don't clobber connection error.
3f95fcdae Don't attempt to set a timeout if we are in an error state.
aacb84b8d Fix typo in makefile.
563b062e3 Accept -nan per the RESP3 spec recommendation.
04c1b5b02 Fix colliding option values
4ca8e73f6 Rework searching for openssl
cd208812f Attempt to find the correct path for openssl.
011f7093c Allow specifying the keepalive interval
e9243d4f7 Cmake static or shared (redis#1160)
1cbd5bc76 Write a version file for the CMake package (redis#1165)
6f5bae8c6 fix typo
acd09461d CMakeLists.txt: respect BUILD_SHARED_LIBS
97fcf0fd1 Add sdevent adapter
ccff093bc Bump dev version for the next release cycle.
c14775b4e Prepare for v1.1.0 GA
f0bdf8405 Add support for nan in RESP3 double (redis#1133)
991b0b0b3 Add an example that calls redisCommandArgv (redis#1140)
a36686f84 CI updates (redis#1139)
8ad4985e9 fix flag reference
7583ebb1b Make freeing a NULL redisAsyncContext a no op.
2c53dea7f Update version in dev branch.
f063370ed Prepare for v1.1.0-rc1
2b069573a CI fixes in preparation of release
e1e9eb40d Add author information to release-drafter template.
afc29ee1a Update for mingw cross compile
ceb8a8815 fixed cpp build error with adapters/libhv.h
3b15a04b5 Fixup of PR734: Coverage of hiredis.c (redis#1124)
c245df9fb CMake corrections for building on Windows (redis#1122)
9c338a598 Fix PUSH handler tests for Redis >= 7.0.5
6d5c3ee74 Install on windows fixes (redis#1117)
68b29e1ad Add timeout support to libhv adapter. (redis#1109)
722e3409c Additional include directory given by pkg-config (redis#1118)
bd9ccb8c4 Use __attribute__ when building with clang on windows
5392adc26 set default SSL certificate directory
560e66486 Minor refactor
d756f68a5 Add libhv example to our standard Makefile
a66916719 Add adapters/libhv
855b48a81 Fix pkgconfig for hiredis_ssl
79ae5ffc6 Fix protocol error (redis#1106)
61b5b299f Use a windows specific keepalive function. (redis#1104)
fce8abc1c Introduce .close method for redisContextFuncs
cfb6ca881 Add REDIS_OPT_PREFER_UNSPEC (redis#1101)
cc7c35ce6 Update documentation to explain redisConnectWithOptions.
bc8d837b7 fix heap-buffer-overflow (redis#957)
ca4a0e850 uvadapter: reduce number of uv_poll_start calls
35d398c90 Fix cmake config path on Linux. CMake config files were installed to `/usr/local/share/hiredis`, which is not recognizable by `find_package()`. I'm not sure why it was set that way. Given the commit introducing it is for Windows, I keep that behavior consistent there, but fix the rest.
10c78c6e1 Add possibility to prefer IPv6, IPv4 or unspecified
1abe0c828 fuzzer: No alloc in redisFormatCommand() when fail
329eaf9ba Fix heap-buffer-overflow issue in redisvFormatCommad
eaae7321c Polling adapter requires sockcompat.h
0a5fa3dde Regression test for off-by-one parsing error
9e174e8f7 Add do while(0) protection for macros
4ad99c69a Rework asSleep to be a generic millisleep function.
75cb6c1ea Do store command timeout in the context for redisSetTimeout (redis#593)
c57cad658 CMake: remove dict.c form hiredis_sources
8491a65a9 Add Github Actions CI workflow for hiredis: Arm, Arm64, 386, windows. (redis#943)
77e4f09ea Merge pull request redis#964 from afcidk/fix-createDoubleObject
9219f7e7c Merge pull request redis#901 from devnexen/illumos_test_fix
810cc6104 Merge pull request redis#905 from sundb/master
df8b74d69 Merge pull request redis#1091 from redis/ssl-error-ub-fix
0ed6cdec3 Fix some undefined behaviour
507a6dcaa Merge pull request redis#1090 from Nordix/subscribe-oom-error
b044eaa6a Copy error to redisAsyncContext when finding subscribe cb
e0200b797 Merge pull request redis#1087 from redis/const-and-non-const-callback
6a3e96ad2 Maintain backward compatibiliy withour onConnect callback.
e7afd998f Merge pull request redis#1079 from SukkaW/drop-macos-10.15-runner
17c8fe079 Merge pull request redis#931 from kristjanvalur/pr2
b808c0c20 Merge pull request redis#1083 from chayim/ck-drafter
367a82bf0 Merge pull request redis#1085 from stanhu/ssl-improve-options-setting
71119a71d Make it possible to set SSL verify mode
dd7979ac1 Merge pull request redis#1084 from stanhu/sh-improve-ssl-docs
c71116178 Improve example for SSL initialization in README.md
5c9b6b571 Release drafter
a606ccf2a CI: use recommended `vmactions/freebsd-vm@v0`
0865c115b Merge pull request redis#1080 from Nordix/readme-corrections
f6cee7142 Fix README typos
06be7ff31 Merge pull request redis#1050 from smmir-cent/fix-cmake-version
7dd833d54 CI: bump macos runner version
f69fac769 Drop `const` on redisAsyncContext in redisConnectCallback Since the callback is now re-entrant, it can call apis such as redisAsyncDisconnect()
005d7edeb Support calling redisAsyncDisconnect from the onConnected callback, by deferring context deletion
6ed060920 Add async regression test for issue redis#931
eaa2a7ee7 Merge pull request redis#932 from kristjanvalur/pr3
2ccef30f3 Add regression test for issue redis#945
4b901d44a Initial async tests
31c91408e Polling adapter and example
8a15f4d65 Merge pull request redis#1057 from orgads/static-name
902dd047f Merge pull request redis#1054 from kristjanvalur/pr08
c78d0926b Merge pull request redis#1074 from michael-grunder/kristjanvalur-pr4
2b115d56c Whitespace
1343988ce Fix typos
47b57aa24 Add some documentation on connect/disconnect callbacks and command callbacks
a890d9ce2 Merge pull request redis#1073 from michael-grunder/kristjanvalur-pr1
f246ee433 Whitespace, style
94c1985bd Use correct type for getsockopt()
5e002bc21 Support failed async connects on windows.
5d68ad2f4 Merge pull request redis#1072 from michael-grunder/fix-redis7-unit-tests
f4b6ed289 Fix tests so they work for Redis 7.0
95a0c1283 Merge pull request redis#1058 from orgads/win64
eedb37a65 Fix warnings on Win64
47c3ecefc Merge pull request redis#1062 from yossigo/fix-push-notification-order
e23d91c97 Merge pull request redis#1061 from yossigo/update-redis-apt
34211ad54 Merge pull request redis#1063 from redis/fix-windows-tests
9957af7e3 Whitelist hiredis repo path in cygwin
b455b3381 Handle push notifications before or after reply.
aed9ce446 Use official repository for redis package.
d7683f35a Merge pull request redis#1047 from Nordix/unsubscribe-handling
7c44a9d7e Merge pull request redis#1045 from Nordix/sds-updates
dd4bf9783 Use the same name for static and shared libraries
ff57c18b9 Embed debug information in windows static lib, rather than create a .pdb file
8310ad4f5 fix cmake version
7123b87f6 Handle any pipelined unsubscribe in async
b6fb548fc Ignore pubsub replies without a channel/pattern
00b82683b Handle overflows as errors instead of asserting
64062a1d4 Catch size_t overflows in sds.c
066c6de79 Use size_t/long to avoid truncation
c6657ef65 Merge branch 'redis:master' into master
50cdcab49 Fix potential fault at createDoubleObject
fd033e983 Remove semicolon after do-while in _EL_CLEANUP
664c415e7 Illumos test fixes, error message difference fot bad hostname test.

git-subtree-dir: deps/hiredis
git-subtree-split: b6a052fe0959dae69e16b9d74449faeb1b70dbe1
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

8 participants