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 Watch/Unwatch #315

Closed
snowbldr opened this issue Jun 21, 2021 · 19 comments · Fixed by #1279
Closed

Support Watch/Unwatch #315

snowbldr opened this issue Jun 21, 2021 · 19 comments · Fixed by #1279
Assignees
Labels
feature type new feature

Comments

@snowbldr
Copy link

I would like to use Watch and Unwatch for full transaction support.

yinqiwen/ardb has a working watch/unwatch impl in case that is useful as a reference.

https://github.com/yinqiwen/ardb

@git-hulk
Copy link
Member

Sure, let's add to the 2.0 plan: https://github.com/KvrocksLabs/kvrocks/projects/1#card-63523056

@git-hulk git-hulk added the feature type new feature label Jun 21, 2021
@201341
Copy link

201341 commented Jul 11, 2022

@git-hulk any update about this feature?

@caipengbo
Copy link
Contributor

Does implementing these two commands have a performance impact? @ShooterIT

@git-hulk
Copy link
Member

Does implementing these two commands have a performance impact? @ShooterIT

Another concern was that we didn't implement multi-keys lock and it's ready now,
maybe we can have a try.

@dirkpetersen
Copy link

I'd also like to get watch/unwatch implemented as it is required by https://juicefs.com/

2022/08/31 18:25:33.286338 juicefs[26231] : checking counter lastCleanupSessions: ERR unknown command watch [base.go:303]
2022/08/31 18:25:35.278170 juicefs[26231] : checking counter lastCleanupFiles: ERR unknown command watch [base.go:389]
2022/08/31 18:25:45.327653 juicefs[26231] : checking counter lastCleanupSessions: ERR unknown command watch [base.go:303]

@git-hulk
Copy link
Member

git-hulk commented Sep 1, 2022

@dirkpetersen Thanks for your feedback

@0xgeert
Copy link

0xgeert commented Jan 8, 2023

Looking forward to this as well. Needed to implement a distributed counter / id generator

@git-hulk
Copy link
Member

git-hulk commented Jan 10, 2023

@0xgeert Thanks for your feedback, will add it to 2023 planning.

@DenizPiri
Copy link
Contributor

We would very much like to see this feature too. It would essentially make it a fully transactional DB. Ideally, it would be nice if it wouldn't affect the performance that much.

Our most common use case would be this:

WATCH key
GET key
MULTI
SET key modifiedValue
EXEC

@git-hulk
Copy link
Member

@DenizPiri Thanks, we are also very eager to have this feature but didn't take much time to dive deep. I think it's time to do it now.

@PragmaTwice
Copy link
Member

PragmaTwice commented Feb 23, 2023

Consider some implemenation methods from scratch:

by snapshots:
It seems to be more "elegant", but is more heavy: make snapshots by each watched key many affect many details in rocksdb,
e.g. the GC process and more.
And it has some different behavior than redis:

set a 1
watch a
set a 1
multi
get a
exec

Redis will return nil, while the snapshot method will continue to execute this redis txn (and return 1).

by command & key tracing:
It seems a little complicated but more clean and has more less influence on storage.

I will try the second way these days, but maybe slow since I have lots of other things to do.

@git-hulk @ShooterIT @caipengbo

@git-hulk
Copy link
Member

git-hulk commented Feb 24, 2023

Thanks for @PragmaTwice driving this design.

Redis will return nil, while the snapshot method will continue to execute this redis txn (and return 1).

So we will check if the watched keys are modified by value, is it right?

@PragmaTwice
Copy link
Member

PragmaTwice commented Feb 24, 2023

So we will check if the watched keys are modified by value, is it right?

We will do this as what redis does: just trace commands by flags and key arguments instead of comparing by value, since the snapshot way is too heavy.

@git-hulk
Copy link
Member

Got it, thanks for your explanation.

@mapleFU
Copy link
Member

mapleFU commented Feb 24, 2023

Personally, I thin using snapshot is too heavy, it may block record gc, and maintaining it is heavy, and logic of snapshot might be heavy.

Using watchkeys as hashmap, and maintaining them seems ok, though it may meets some concurrent and performance related issue. I think this is ok and better.

Another way is, like replication, find these keys and ops from WAL.

Comparing to check keys in commands and wal, the first one may not has best performance, but it's easy and real-time. The later may have better performance, but user may get the message a bit later.

@git-hulk git-hulk moved this from To Do to In Progress in Kvrocks 2024 Roadmap Mar 6, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Kvrocks 2024 Roadmap Mar 7, 2023
@git-hulk
Copy link
Member

git-hulk commented Mar 7, 2023

Hi, folks. This watch/unwatch command has been merged into unstable, welcome to have a try if you are interested.

@PragmaTwice PragmaTwice self-assigned this Mar 11, 2023
@DenizPiri
Copy link
Contributor

@git-hulk We started utilizing this in one of our projects. We migrated from redis. It has been running for the last 2 days. It seems to be working very well so far. 🚀 Doing around 1500ops/sec, mostly single key WATCH -> GET -> MULTI -> SET -> EXEC commands.

If we were to use WATCH/UNWATCH support more widely in our other projects too, assuming that 99.9% of the time transactions will execute without any conflicts. Should we expect degraded performance, possibly because of a lock contention?

@git-hulk
Copy link
Member

@DenizPiri Thanks for your feedback and give the credit to @PragmaTwice great work.

If we were to use WATCH/UNWATCH support more widely in our other projects too, assuming that 99.9% of the time transactions will execute without any conflicts. Should we expect degraded performance, possibly because of a lock contention?

Yes, it may have a performance issue since the MULTI-EXEC is an exclusive operation even though most of the commands are no conflicts. I'm also seeking a way to improve this situation, but you can have a simple benchmark before using it.

@PragmaTwice
Copy link
Member

PragmaTwice commented Mar 14, 2023

If we were to use WATCH/UNWATCH support more widely in our other projects too, assuming that 99.9% of the time transactions will execute without any conflicts. Should we expect degraded performance, possibly because of a lock contention?

I think compared to the performance downgrade of WATCH, the downgrade of MULTI-EXEC may be more large. So if you already use some MULTI-EXEC pattern, the runtime cost to use WATCH may be less.

You can try it in your case and share performance data with us if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature type new feature
Projects
Development

Successfully merging a pull request may close this issue.

9 participants