Skip to content

Add API to allow the MaxCost of an existing cache to be updated.#200

Merged
martinmr merged 6 commits intomasterfrom
martinmr/update-cost
Oct 7, 2020
Merged

Add API to allow the MaxCost of an existing cache to be updated.#200
martinmr merged 6 commits intomasterfrom
martinmr/update-cost

Conversation

@martinmr
Copy link
Contributor

@martinmr martinmr commented Oct 1, 2020

This change is Reviewable

cache_test.go Outdated
// Update the max cost of the cache and retry.
c.UpdateMaxCost(1000)
require.Equal(t, int64(1000), c.MaxCost())
if c.Set(1, 1, 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if this set fail? Shouldn't we do a require.True (t, c.Set(...))

cache_test.go Outdated
})
require.NoError(t, err)
require.Equal(t, int64(10), c.MaxCost())
if c.Set(1, 1, 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if this set fail? Shouldn't we do a require.True (t, c.Set(...))

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Looks alright to me. Will wait for @jarifibrahim 's review.

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jarifibrahim)


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

Previously, jarifibrahim (Ibrahim Jarif) wrote…

what if this set fail? Shouldn't we do a require.True (t, c.Set(...))

Done.


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

Previously, jarifibrahim (Ibrahim Jarif) wrote…

what if this set fail? Shouldn't we do a require.True (t, c.Set(...))

Done.

@martinmr martinmr merged commit 9739cfa into master Oct 7, 2020
@martinmr martinmr deleted the martinmr/update-cost branch October 7, 2020 16:43
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.

3 participants