Skip to content

Improve handling of updated items#168

Merged
jarifibrahim merged 6 commits intomasterfrom
ibrahim/update-bug
Jun 30, 2020
Merged

Improve handling of updated items#168
jarifibrahim merged 6 commits intomasterfrom
ibrahim/update-bug

Conversation

@jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jun 29, 2020 •
edited
Loading

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

jarifibrahim
jarifibrahim commented Jun 29, 2020
View reviewed changes
martinmr
martinmr suggested changes Jun 29, 2020
View reviewed changes
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
Closed
@jarifibrahim jarifibrahim mentioned this pull request Jun 30, 2020
Closed
martinmr
martinmr approved these changes Jun 30, 2020
View reviewed changes
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 jarifibrahim mentioned this pull request Jun 30, 2020
Merged
jarifibrahim pushed a commit to dgraph-io/badger that referenced this pull request Jun 30, 2020
Update ristretto to commit f66de99 (#1391)
09dfa66 
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
Update ristretto to commit f66de99 (#1391)
25fd0ef 
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
Update ristretto to commit f66de99 (#1391)
0e9748b 
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
Update ristretto to commit f66de99 (#1391)
cf51ce1 
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

Reviewers

@all-seeing-code all-seeing-code Awaiting requested review from all-seeing-code

@karlmcguire karlmcguire Awaiting requested review from karlmcguire

@manishrjain manishrjain Awaiting requested review from manishrjain

1 more reviewer

@martinmr martinmr martinmr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update returns false even when the item was updated

2 participants

@jarifibrahim @martinmr