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

Improve handling of updated items #168

Merged
merged 6 commits into from
Jun 30, 2020
Merged

Improve handling of updated items #168

merged 6 commits into from
Jun 30, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jun 29, 2020

When an item is updated, the existing item was updated in the store but we might return false.
Update happens here

ristretto/cache.go

Lines 229 to 231 in a093fe6

if c.store.Update(i) {
i.flag = itemUpdate
}

and the false is returned here

ristretto/cache.go

Lines 232 to 239 in a093fe6

// Attempt to send item to policy.
select {
case c.setBuf <- i:
return true
default:
c.Metrics.add(dropSets, keyHash, 1)
return false
}

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

Fixes #167


This change is Reviewable

cache.go Show resolved Hide resolved
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

A couple of minor changes but it :lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anurags92, @jarifibrahim, @karlmcguire, and @manishrjain)


cache_test.go, line 620 at r1 (raw file):

}

func TestUpdateBug(t *testing.T) {

TestDropUpdates or something more descriptive.


cache_test.go, line 642 at r1 (raw file):

		}

		// This is important.

comment should say why.

@martinmr martinmr mentioned this pull request Jun 29, 2020
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anurags92, @jarifibrahim, @karlmcguire, and @manishrjain)

@jarifibrahim jarifibrahim merged commit f66de99 into master Jun 30, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/update-bug branch June 30, 2020 15:40
jarifibrahim pushed a commit to dgraph-io/badger that referenced this pull request Jun 30, 2020
The update brings following changes from ristretto into badger

```
f66de99 Improve handling of updated items (dgraph-io/ristretto#168)
aec7994 Fix flaky TestCacheDel test (dgraph-io/ristretto#169)
a093fe6 Add benchmark plots to the project repo (dgraph-io/ristretto#166)
49dc42c Add Anurag as codeowner (dgraph-io/ristretto#158)
7a3f2d3 z: use MemHashString and xxhash.Sum64String (dgraph-io/ristretto#153)
9c31bb2 Check conflict key before updating expiration map. (dgraph-io/ristretto#154)
62cb731 Fix race condition in Cache.Clear (dgraph-io/ristretto#133)
9af1934 Docs and whitespace changes for readability. (dgraph-io/ristretto#143)
dbc185e Add changelog. (dgraph-io/ristretto#142)
ff325ad Remove key from policy after TTL eviction (dgraph-io/ristretto#130)
2dd5ff5 Use the require library in all the tests. (dgraph-io/ristretto#128)
7c48141 Use require in all tests in cache_test.go (dgraph-io/ristretto#127)
51e97ad Sets with TTL (dgraph-io/ristretto#122)
d3e7c37 Add benchmarks for math.rand and fastrand (dgraph-io/ristretto#118)
593823e Integrate fixes from PR dgraph-io/ristretto#91. (dgraph-io/ristretto#126)
29b4dd7 Fix comments. (dgraph-io/ristretto#123)
ddf345c Removed workflows directory. (dgraph-io/ristretto#124)
eb104d0 Add martinmr to CODEOWNERS file. (dgraph-io/ristretto#125)
```
jarifibrahim pushed a commit to dgraph-io/badger that referenced this pull request Jul 6, 2020
The update brings following changes from ristretto into badger

```
f66de99 Improve handling of updated items (dgraph-io/ristretto#168)
aec7994 Fix flaky TestCacheDel test (dgraph-io/ristretto#169)
a093fe6 Add benchmark plots to the project repo (dgraph-io/ristretto#166)
49dc42c Add Anurag as codeowner (dgraph-io/ristretto#158)
7a3f2d3 z: use MemHashString and xxhash.Sum64String (dgraph-io/ristretto#153)
9c31bb2 Check conflict key before updating expiration map. (dgraph-io/ristretto#154)
62cb731 Fix race condition in Cache.Clear (dgraph-io/ristretto#133)
9af1934 Docs and whitespace changes for readability. (dgraph-io/ristretto#143)
dbc185e Add changelog. (dgraph-io/ristretto#142)
ff325ad Remove key from policy after TTL eviction (dgraph-io/ristretto#130)
2dd5ff5 Use the require library in all the tests. (dgraph-io/ristretto#128)
7c48141 Use require in all tests in cache_test.go (dgraph-io/ristretto#127)
51e97ad Sets with TTL (dgraph-io/ristretto#122)
d3e7c37 Add benchmarks for math.rand and fastrand (dgraph-io/ristretto#118)
593823e Integrate fixes from PR dgraph-io/ristretto#91. (dgraph-io/ristretto#126)
29b4dd7 Fix comments. (dgraph-io/ristretto#123)
ddf345c Removed workflows directory. (dgraph-io/ristretto#124)
eb104d0 Add martinmr to CODEOWNERS file. (dgraph-io/ristretto#125)
```

(cherry picked from commit 09dfa66)
jarifibrahim pushed a commit to dgraph-io/badger that referenced this pull request Oct 2, 2020
The update brings following changes from ristretto into badger

```
f66de99 Improve handling of updated items (dgraph-io/ristretto#168)
aec7994 Fix flaky TestCacheDel test (dgraph-io/ristretto#169)
a093fe6 Add benchmark plots to the project repo (dgraph-io/ristretto#166)
49dc42c Add Anurag as codeowner (dgraph-io/ristretto#158)
7a3f2d3 z: use MemHashString and xxhash.Sum64String (dgraph-io/ristretto#153)
9c31bb2 Check conflict key before updating expiration map. (dgraph-io/ristretto#154)
62cb731 Fix race condition in Cache.Clear (dgraph-io/ristretto#133)
9af1934 Docs and whitespace changes for readability. (dgraph-io/ristretto#143)
dbc185e Add changelog. (dgraph-io/ristretto#142)
ff325ad Remove key from policy after TTL eviction (dgraph-io/ristretto#130)
2dd5ff5 Use the require library in all the tests. (dgraph-io/ristretto#128)
7c48141 Use require in all tests in cache_test.go (dgraph-io/ristretto#127)
51e97ad Sets with TTL (dgraph-io/ristretto#122)
d3e7c37 Add benchmarks for math.rand and fastrand (dgraph-io/ristretto#118)
593823e Integrate fixes from PR dgraph-io/ristretto#91. (dgraph-io/ristretto#126)
29b4dd7 Fix comments. (dgraph-io/ristretto#123)
ddf345c Removed workflows directory. (dgraph-io/ristretto#124)
eb104d0 Add martinmr to CODEOWNERS file. (dgraph-io/ristretto#125)
```
manishrjain pushed a commit to outcaste-io/outserv that referenced this pull request Jul 6, 2022
The update brings following changes from ristretto into badger

```
f66de99 Improve handling of updated items (dgraph-io/ristretto#168)
aec7994 Fix flaky TestCacheDel test (dgraph-io/ristretto#169)
a093fe6 Add benchmark plots to the project repo (dgraph-io/ristretto#166)
49dc42c Add Anurag as codeowner (dgraph-io/ristretto#158)
7a3f2d3 z: use MemHashString and xxhash.Sum64String (dgraph-io/ristretto#153)
9c31bb2 Check conflict key before updating expiration map. (dgraph-io/ristretto#154)
62cb731 Fix race condition in Cache.Clear (dgraph-io/ristretto#133)
9af1934 Docs and whitespace changes for readability. (dgraph-io/ristretto#143)
dbc185e Add changelog. (dgraph-io/ristretto#142)
ff325ad Remove key from policy after TTL eviction (dgraph-io/ristretto#130)
2dd5ff5 Use the require library in all the tests. (dgraph-io/ristretto#128)
7c48141 Use require in all tests in cache_test.go (dgraph-io/ristretto#127)
51e97ad Sets with TTL (dgraph-io/ristretto#122)
d3e7c37 Add benchmarks for math.rand and fastrand (dgraph-io/ristretto#118)
593823e Integrate fixes from PR dgraph-io/ristretto#91. (dgraph-io/ristretto#126)
29b4dd7 Fix comments. (dgraph-io/ristretto#123)
ddf345c Removed workflows directory. (dgraph-io/ristretto#124)
eb104d0 Add martinmr to CODEOWNERS file. (dgraph-io/ristretto#125)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Update returns false even when the item was updated
2 participants