Add remove reason signal to OnRemove callback #80

jshufro · 2018-04-05T19:53:52Z

I'm still pretty new to golang, so this PR probably has a C-like odor to it (especially with regard to the bitmasks). The general purpose is to indicate in the OnRemove callback why a key is being removed. There are 3 reasons, represented as bitmasks by leftshifting iota in a const expression. I'm not sure if there's a more go-like approach, but I'd like to see support for this feature so I thought I'd open a PR and get a discussion going.

As for the API- I've made a small breaking change. Users of the library will have to add a parameter to their OnRemove callbacks, which can be ignored. If it's preferable to add new functions instead of changing the signature if existing ones, I'd be happy to adapt the code.

coveralls · 2018-04-05T19:55:32Z

Coverage increased (+0.2%) to 90.223% when pulling 7385726 on jshufro:tracking into e3e2429 on allegro:master.

siennathesane

Overall it looks good, I like there is a reason. I would ask you fix the iota comments to make it more idiomatic and then either add a test to cover Expired or implement it in the same test.

siennathesane · 2018-04-05T19:59:16Z

bigcache.go

+type RemoveReason int32
+const (
+	// Expired means the key is past its LifeWindow.
+	Expired    = 1 << iota


I don't really see a reason to left shift an iota, tbh. You can just call iota like this:

const ( Expired = iota NoSpace Deleted )

It will do the same thing. :)

from my understanding, what you're suggesting will assign the sequence of values as 0, 1, 2

However, for bitmasks, the correct sequence is 1,2,4,8,16 etc
since the binary representation of 4 is, for example, 0100, you can combine these masks with the bitwise | to store multiple bools on the same integer. Thus, Expired | Deleted actually equals 0101, which is what lets me store just one integer instead of several bools

Mmmm while I do appreciate the optimisation, I am going to disagree and say while it is an integer optimization, the benefit of the optimisation underweighs against the idiotmatic use of iota. When you filed the PR, you made this statement:

I'm still pretty new to golang, so this PR probably has a C-like odor to it

Bit shifting an iota is something you shouldn't really be doing in idiomatic Go as a lot of the reason for iota was to provide an enumerator that you don't have to manage or think about - the runtime manages it.

I'm going to defer to @janisz, but my stance is that bit shifting an iota is over-engineering and not idiomatic.

I'm fine changing the approach to be a list of bools that say basically blockOnRemoveForSpace, blockOnRemoveForExpiration, blockOnRemoveForDeletion- the downside is that in the Set path, where the keys are evicted, we have to do a cascading "if reason == eviction && cache.blockOnRemoveForEviction == false { cache.OnRemove(reason) } else if ...."

In C we always avoid doing long, sequential boolean checks using bitmaps. I guess in Go it's just about choosing where the mess goes- either you bitshift an iota (or, if you'd prefer, we can assign constant values 1 2 4 8 16 etc)- or you check a series of booleans sequentially and have a bunch of branches in a single function.

I'll wait to hear @janisz opinion before making any changes, though.

In C we always avoid doing long, sequential boolean checks using bitmaps.

I agree, but this is Go, you can always do:

switch reason { case Expired: // case NoSpace: // case Deleted: // }

Which is both cleaner logic as well as being idiomatic. More lines of code for idiomacy is better.

The switch is definitely cleaner- but it still requires 3x variables on the config instead of one, for the filter

siennathesane · 2018-04-05T19:59:29Z

bigcache.go

+	Expired    = 1 << iota
+	// NoSpace means the key is the oldest and the cache size was at its maximum when Set was called, or the
+	// entry exceeded the maximum shard size.
+	NoSpace    = 1 << iota


siennathesane · 2018-04-05T19:59:35Z

bigcache.go

+	// entry exceeded the maximum shard size.
+	NoSpace    = 1 << iota
+	// Deleted means Delete was called and this key was removed as a result.
+	Deleted    = 1 << iota


siennathesane · 2018-04-05T20:03:38Z

bigcache_test.go

+		MaxEntriesInWindow: 1,
+		MaxEntrySize:       256,
+		OnRemove:           onRemove,
+		OnRemoveFilter:     Deleted | NoSpace,


Should this be || once you fix the bit shifting?

siennathesane · 2018-04-05T20:04:10Z

bigcache_test.go

@@ -203,6 +204,39 @@ func TestOnRemoveCallback(t *testing.T) {
 	assert.True(t, onRemoveInvoked)
 }

+func TestOnRemoveFilter(t *testing.T) {


Need to add another test that would cover Expired as well. Or implement Expired in this test. :)

TestOnRemoveCallback asserts the reason is Expired- I can add another test that makes sure it works in the Filter, but we're covered by the expisting one which fails if we get a callback on expiration

cristaloleg

I have some doubts regarding breaking change around OnRemove, maybe we should add OnRemoveExt(Ext from Extended) ? So we will save backcompatibility, but add a new feature. WDYT?

siennathesane · 2018-04-05T20:09:42Z

Also, Travis is failing due to formatting, so make sure to run go fmt on files you change so it doesn't error out on linting. 😄

siennathesane · 2018-04-05T20:15:42Z

I really like this change, I would posit there is a path forward via versioning:

Current: OnRemove + OnRemoveExt
Current+1: OnRemove with deprecation notice + OnRemoveExt
Current+2: OnRemoveExt

Go's recommended deprecation path:

Sometimes a struct field, function, type, or even a whole package becomes redundant or unnecessary, but must be kept for compatibility with existing programs. To signal that an identifier should not be used, add a paragraph to its doc comment that begins with "Deprecated:" followed by some information about the deprecation. There are a few examples in the standard library.

I do agree we shouldn't break backwards compatibility but this is a nice change.

jshufro · 2018-04-06T16:31:13Z

I've made the recommended changes with regard to the API breakage. It should be easy to deprecate OnRemove when the time comes.

The only thing worth noting is that OnRemoveExt will be ignored as long as OnRemove is provided, as users shouldn't use both. I've updated the comments/docs to reflect this

siennathesane · 2018-04-07T02:00:01Z

@jshufro can you please fix the bit shifting iotas? The rest looks good. :)

jshufro · 2018-04-07T04:38:39Z

@mxplusb I replied inline earlier - #80 (comment)

janisz · 2018-04-09T09:12:48Z

README.md

+		// for the new entry, or because delete was called. A bitmask representing the reason will be passed through.
+		// Default value is nil which means no callback and it prevents from unwrapping the oldest entry.
+		// Ignored if OnRemove is specified.
+		OnRemoveExt func(key string, entry []byte, reason RemoveReason)


I'd prefer different name OnRemoveWithReason

jshufro · 2018-04-16T17:54:09Z

sorry for the radio silence- i'll get back to this shortly.

I actually have a decent idea of how to keep the single bitmask for the filter without shifting the constants

siennathesane · 2018-04-16T22:02:15Z

@jshufro excited to see what you come up with!

jshufro · 2018-04-20T17:48:44Z

I pushed up a change that does the following:

no longer bitshift by iota, but instead bitshift when reading/writing the filter mask
add a variadric function to set the filter mask
edit: the function introduces the fluent pattern for config.go- there are no other 'setters', so I'm curious to hear if this approach is acceptable.
rename OnRemoveExt to OnRemoveWithReason
and obviously update docs/comments

cristaloleg · 2018-04-21T10:54:53Z

bigcache.go

+
+const (
+	// Expired means the key is past its LifeWindow.
+	Expired = iota


Expired RemoveReason = iota, without an explicit type it will be int.
See example https://play.golang.org/p/mZZdMaI92cI

Neat. I'll update.

cristaloleg · 2018-04-21T10:58:01Z

bigcache_test.go

+	assert.False(t, onRemoveInvoked)
+
+	// and when
+	cache.Delete("key2")


onRemoveInvoked should be set to false before this line, isn't it?

It's initialized to false on L246 and asserted to still be false on L266

I can explicitly set it to false here if it makes things clearer?

Ah, indeed. So just ignore my comment, sorry :)

siennathesane · 2018-04-22T15:27:23Z

@jshufro lgtm, thanks for the change!

jshufro · 2018-04-22T18:48:43Z

Had to amend the commit to add a gpg signature

jshufro · 2018-04-23T14:55:35Z

I signed this with a key for the wrong email. I'll resign it.

jshufro · 2018-04-23T15:04:06Z

Fixed the gpg signature. I promise this is the last update

siennathesane · 2018-04-23T22:20:39Z

Merged, thanks for the contribution!

siennathesane requested changes Apr 5, 2018

View reviewed changes

cristaloleg requested changes Apr 5, 2018

View reviewed changes

jshufro force-pushed the tracking branch from 7ac9fdb to 92adeca Compare April 5, 2018 20:13

jshufro force-pushed the tracking branch from 92adeca to 4faae0c Compare April 6, 2018 16:02

janisz reviewed Apr 9, 2018

View reviewed changes

jshufro force-pushed the tracking branch from 4faae0c to 389dc85 Compare April 20, 2018 17:46

cristaloleg reviewed Apr 21, 2018

View reviewed changes

jshufro force-pushed the tracking branch from 389dc85 to 0071311 Compare April 21, 2018 19:56

cristaloleg approved these changes Apr 22, 2018

View reviewed changes

siennathesane previously approved these changes Apr 22, 2018

View reviewed changes

jshufro dismissed siennathesane’s stale review via 3d7837b April 22, 2018 18:47

jshufro force-pushed the tracking branch from 0071311 to 3d7837b Compare April 22, 2018 18:47

cristaloleg approved these changes Apr 22, 2018

View reviewed changes

janisz previously approved these changes Apr 23, 2018

View reviewed changes

druminski previously approved these changes Apr 23, 2018

View reviewed changes

Add remove reason signal to OnRemove callback

7385726

jshufro dismissed stale reviews from druminski and janisz via 7385726 April 23, 2018 15:02

jshufro force-pushed the tracking branch from 3d7837b to 7385726 Compare April 23, 2018 15:02

janisz approved these changes Apr 23, 2018

View reviewed changes

cristaloleg approved these changes Apr 23, 2018

View reviewed changes

siennathesane merged commit 7385726 into allegro:master Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remove reason signal to OnRemove callback #80

Add remove reason signal to OnRemove callback #80

jshufro commented Apr 5, 2018

coveralls commented Apr 5, 2018 •

edited

siennathesane left a comment

siennathesane Apr 5, 2018

jshufro Apr 5, 2018 •

edited

siennathesane Apr 9, 2018

jshufro Apr 9, 2018

siennathesane Apr 9, 2018

jshufro Apr 9, 2018 •

edited

siennathesane Apr 5, 2018

siennathesane Apr 5, 2018

siennathesane Apr 5, 2018

siennathesane Apr 5, 2018

jshufro Apr 5, 2018

cristaloleg left a comment

siennathesane commented Apr 5, 2018

siennathesane commented Apr 5, 2018 •

edited

jshufro commented Apr 6, 2018

siennathesane commented Apr 7, 2018

jshufro commented Apr 7, 2018

janisz Apr 9, 2018

jshufro commented Apr 16, 2018

siennathesane commented Apr 16, 2018

jshufro commented Apr 20, 2018 •

edited

cristaloleg Apr 21, 2018

jshufro Apr 21, 2018

cristaloleg Apr 21, 2018

jshufro Apr 21, 2018

cristaloleg Apr 22, 2018

siennathesane commented Apr 22, 2018

jshufro commented Apr 22, 2018

jshufro commented Apr 23, 2018

jshufro commented Apr 23, 2018

siennathesane commented Apr 23, 2018

Add remove reason signal to OnRemove callback #80

Add remove reason signal to OnRemove callback #80

Conversation

jshufro commented Apr 5, 2018

coveralls commented Apr 5, 2018 • edited

siennathesane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshufro Apr 5, 2018 • 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

jshufro Apr 9, 2018 • 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

cristaloleg left a comment

Choose a reason for hiding this comment

siennathesane commented Apr 5, 2018

siennathesane commented Apr 5, 2018 • edited

jshufro commented Apr 6, 2018

siennathesane commented Apr 7, 2018

jshufro commented Apr 7, 2018

Choose a reason for hiding this comment

jshufro commented Apr 16, 2018

siennathesane commented Apr 16, 2018

jshufro commented Apr 20, 2018 • 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

siennathesane commented Apr 22, 2018

jshufro commented Apr 22, 2018

jshufro commented Apr 23, 2018

jshufro commented Apr 23, 2018

siennathesane commented Apr 23, 2018

coveralls commented Apr 5, 2018 •

edited

jshufro Apr 5, 2018 •

edited

jshufro Apr 9, 2018 •

edited

siennathesane commented Apr 5, 2018 •

edited

jshufro commented Apr 20, 2018 •

edited