35 Added the support for FILTERKEYS #84

MayukhSobo · 2022-11-02T21:44:01Z

Fixes #35

It seems I have added too much but there is a reason why something so trivial should take this much time. Apologies for such an extreme level of changes and please allow me to explain it.

FILTERKEYS is supposed to work on multiple keys and keeping in mind that we are not using any special data structure to store the keys in any ORDERED manner and the fact the regex can be complete random, the best time complexity that we can achieve is O(n) via iterating over all the keys.
This leaves me with no choice but either implement the FILTERKEYS trivially or use sheer brute force of concurrency to optimise the performance.
I choose the second option. This also goes with the thought that @arpitbbhayani had to make individual commands as concurrent but not the multiple command just to keep it simple for now.
With the introduction of go routine worker pool, I had to introduce synchronisation issues inevitably. To fight this there are two ways. Either I use normal golang maps with Mutex or use sync.Map from go1.9.
Go has a weird property. The map is not thread safe. Even if multiple goroutines are working on individual keys. It is not even thread safe if we are reading on keys A and B from one goroutine and writing on C and D from another goroutines. That's weird. Maps are only safe iff we initialise the map before all the go routine starts and they don't change before reading ends.
Here this is not the case because we are also deleting the expired keys while preforming read keys for example. This means the read in this scenario is also not safe.
Implementing mutex along with map is good but cumbersome. People usually use sync.Mutex but there is also sync.RWMutex. Nevertheless, performance of map with mutex doesn't scale well in my experience and has a lot of implementation blockers.
sync.Map was introduced keeping in mind this issue in go 1.9. It scales EXCEPTIONALLY better if the vertical scaling of the system is considerable.
sync.Map has a problem. We can't use len function on it. So I had to create my own custom Storage(DiceKVStore) with an atomic count in it.
Because I had to introduce sync.Map and goroutine pool (can explain why not simple go routine if needed), I was mistakenly introducing a lot of cyclic dependency in project. In simpler word, package A is importing package B and package B is importing package A. This reminded me that the current design is very tightly coupled. So I had to introduce a lot of packages.

I know I have introduced a lot of changes and I also know huge changes in a single PR is a bad practise but I couldn't stay away from optimising it. It took me a lot of time to rewire the whole thing. I would request the reviewers to review the code and give pointers. If you guys decide to merge this, I shall be ready to work along with other contributors to adopt change that happened in between. If not, you can put it in a separate experimental branch or else worst option, I can continue it on my fork.

Thanks
Mayukh

arpitbbhayani · 2022-11-03T18:42:38Z

IStorageEngines/diceKV.go

+	GetCount() uint64
+	GetStorage() *sync.Map


Let's rename it to Count and Storage ... more go-like. and as discussed, the Storage should return an iterator - a function that upon invocation returns the next KV pair.

arpitbbhayani · 2022-11-03T18:42:48Z

IStorageEngines/redisKV.go

+package istorageengines
+
+// This is for future usage to use Redis Storage
+// as the main storage engine instead of KV


let's remove this file.

arpitbbhayani · 2022-11-03T18:43:10Z

IStorageEngines/diceKV.go

+package istorageengines
+


name of the package should be storage_engines

arpitbbhayani · 2022-11-03T18:43:37Z

IStorageEngines/diceKV.go

+	"github.com/dicedb/dice/object"
+)
+
+type IKVStorage interface {


rename the interface and keep it StorageEngine

arpitbbhayani · 2022-11-03T18:43:53Z

core/aof.go

@@ -6,28 +6,31 @@ import (
 	"os"
 	"strings"

+	dbEngine "github.com/dicedb/dice/IStorageEngines"


dbEngine -> storage_engine

arpitbbhayani · 2022-11-03T18:44:27Z

core/aof.go

 	"github.com/dicedb/dice/config"
+	"github.com/dicedb/dice/object"


instead of creating another package for object see if we can fit it in storage_engine or core

arpitbbhayani · 2022-11-03T18:45:39Z

core/comm.go

@@ -26,13 +28,13 @@ func (c *Client) TxnBegin() {
 	c.isTxn = true
 }

-func (c *Client) TxnExec() []byte {
+func (c *Client) TxnExec(dh *handlers.DiceKVstoreHandler) []byte {


Instead of calling it handler, let's name our storage engine and use that here.

I propose the name - ozone

arpitbbhayani · 2022-11-03T18:47:12Z

expiry/expire.go

+package expiry
+
+import (
+	dbEngine "github.com/dicedb/dice/IStorageEngines"
+	"github.com/dicedb/dice/object"
+)
+
+func expireSample(dh dbEngine.IKVStorage) float32 {
+	var limit int = 20
+	var expiredCount int = 0
+	dh.GetStorage().Range(func(k, v interface{}) bool {
+		key := k.(string)
+		value := v.(*object.Obj)
+		// fmt.Printf("The key is : %v and the value is %v\n", key, *value)
+		limit--
+		if object.GetDiceExpiryStore().HasExpired(value) {
+			dh.Del(key)
+			expiredCount++
+		}
+		if limit == 0 {
+			return true
+		}
+		return true
+	})
+	return float32(expiredCount) / float32(limit)
+}
+
+// Deletes all the expired keys - the active way
+// Sampling approach: https://redis.io/commands/expire/
+func DeleteExpiredKeys(dh dbEngine.IKVStorage) {


let expiry be part of the storage engine implementation and expose it as an interface for an explicit trigger.

arpitbbhayani · 2022-11-03T18:47:39Z

handlers/diceKVOps.go

+type DiceKVstoreHandler struct {
+	object.DiceKVstore
+}


rename it to our storage engine ozone

arpitbbhayani · 2022-11-03T18:48:52Z

pool/dicePool.go

+	// Max number of workers 256 * 1024
+	DefaultPoolSize = 1 << 18
+
+	// If we should wait when pool workers ain't available
+	// false make it won't wait
+	Nonblocking = false
+
+	// ExpiryDuration is the interval time to clean up those expired workers.
+	ExpiryDuration = 10 * time.Second


DefaultPoolSize -> MaxConcurrency and let the default value be part of the config.
Expiry duration can also be part of the config. Nonblocking should always be true. Don't see a reason for it to be configurable.

35 Added the support for FILTERKEYS

a0bbb36

arpitbbhayani requested changes Nov 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

35 Added the support for FILTERKEYS #84

35 Added the support for FILTERKEYS #84

MayukhSobo commented Nov 2, 2022 •

edited

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

arpitbbhayani Nov 3, 2022

		"github.com/dicedb/dice/config"
		"github.com/dicedb/dice/object"

35 Added the support for FILTERKEYS #84

Are you sure you want to change the base?

35 Added the support for FILTERKEYS #84

Conversation

MayukhSobo commented Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MayukhSobo commented Nov 2, 2022 •

edited