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

Update returns false even when the item was updated #167

Closed
jarifibrahim opened this issue Jun 29, 2020 · 0 comments · Fixed by #168
Closed

Update returns false even when the item was updated #167

jarifibrahim opened this issue Jun 29, 2020 · 0 comments · Fixed by #168
Labels
status/accepted We accept to work on it.

Comments

@jarifibrahim
Copy link
Contributor

The bug was seen in dgraph-io/badger#1350 .
See dgraph-io/badger#1350 (comment) for a detailed explanation.

The following TestUpdateBug test fails on current master a093fe6

diff --git a/cache.go b/cache.go
index e48a48a..c5e19f0 100644
--- a/cache.go
+++ b/cache.go
@@ -29,7 +29,7 @@ import (
 	"github.com/dgraph-io/ristretto/z"
 )
 
-const (
+var (
 	// TODO: find the optimal value for this or make it configurable
 	setBufSize = 32 * 1024
 )
diff --git a/cache_test.go b/cache_test.go
index c1dc745..7c0bd9b 100644
--- a/cache_test.go
+++ b/cache_test.go
@@ -1,7 +1,9 @@
 package ristretto
 
 import (
+	"fmt"
 	"math/rand"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -614,3 +616,54 @@ func init() {
 	// Set bucketSizeSecs to 1 to avoid waiting too much during the tests.
 	bucketDurationSecs = 1
 }
+
+func TestUpdateBug(t *testing.T) {
+	originalSetBugSize := setBufSize
+	defer func() { setBufSize = originalSetBugSize }()
+	test := func() {
+		droppedMap := make(map[int]struct{})
+		lastEvictedSet := int64(-1)
+
+		var err error
+		handler := func(_ interface{}, value interface{}) {
+			v := value.(string)
+			lastEvictedSet, err = strconv.ParseInt(string(v), 10, 32)
+			require.NoError(t, err)
+			_, ok := droppedMap[int(lastEvictedSet)]
+			if ok {
+				panic(fmt.Sprintf("val = %+v was dropped but got evicted. Dropped items: %+v\n", lastEvictedSet, droppedMap))
+			}
+		}
+
+		// This is important.
+		setBufSize = 10
+
+		c, err := NewCache(&Config{
+			NumCounters: 100,
+			MaxCost:     10,
+			BufferItems: 64,
+			Metrics:     true,
+			OnEvict: func(_, _ uint64, value interface{}, _ int64) {
+				handler(nil, value)
+			},
+		})
+		require.NoError(t, err)
+
+		for i := 0; i < 5*setBufSize; i++ {
+			v := fmt.Sprintf("%0100d", i)
+			// We're updating the same key.
+			if !c.Set(0, v, 1) {
+				time.Sleep(time.Microsecond)
+				// fmt.Println("failed", i)
+				droppedMap[i] = struct{}{}
+			}
+		}
+		time.Sleep(time.Millisecond)
+		require.True(t, c.Set(1, nil, 10))
+		c.Close()
+	}
+	// Run the test 100 times.
+	for i := 0; i < 100; i++ {
+		test()
+	}
+}
@lgalatin lgalatin added the status/accepted We accept to work on it. label Jun 29, 2020
jarifibrahim pushed a commit that referenced this issue Jun 30, 2020
When an item is updated, the existing item was updated in the `store` but we
might return false.  Update happens here
https://github.com/dgraph-io/ristretto/blob/a093fe661329ba68a99a9c3a63674992355f40b1/cache.go#L229-L231
and the false is returned here
https://github.com/dgraph-io/ristretto/blob/a093fe661329ba68a99a9c3a63674992355f40b1/cache.go#L232-L239

With this PR, we will always return true for updates, even if the policy wasn't updated.
See #167 for more details.

Fixes #167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/accepted We accept to work on it.
Development

Successfully merging a pull request may close this issue.

2 participants