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

inital prep for v3. #245

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

inital prep for v3. #245

wants to merge 5 commits into from

Conversation

siennathesane
Copy link
Collaborator

This is the base branch to make progress on the v3 work and start getting things in place.

Signed-off-by: Mike Lloyd mike@reboot3times.org

Mike Lloyd and others added 2 commits November 4, 2020 14:14
Signed-off-by: Mike Lloyd <mike@reboot3times.org>
* Fix iterator styling issues (#247)

Issues reported in #246

* fix #160 (#246)

* Update to latest golang (#248)

Co-authored-by: Oleg Kovalov <iamolegkovalov@gmail.com>

* inital prep for v3.

Signed-off-by: Mike Lloyd <mike@reboot3times.org>

* Use uint64 intead of uint32

There are posibility we run into a problem of int32 overflow.
To prevent this let's use uint64 everywhere.

https://github.com/allegro/bigcache/blob/21e5ca5c3d539f94e8dc563350acd97c5400154f/shard.go#L138

Fixes: #148

* Fix CI

* Do not run on 1.13

* Do not run long test

* Optimze append (#249)

* Add Benchmark for append

* Optimize Append and halve byte copies

* Optimize Append by reducing allocs

* Optimize Append by reducing allocs

* Reduces allocs from test construct

Co-authored-by: Fabian Gärtner <fabian.gaertner@check24.de>

Co-authored-by: S@P <lorenzotorracchi@mail.com>
Co-authored-by: Oleg Kovalov <iamolegkovalov@gmail.com>
Co-authored-by: Mike Lloyd <mlloyd@rts.com>
Co-authored-by: Fabianexe <Fabianexe@gmail.com>
Co-authored-by: Fabian Gärtner <fabian.gaertner@check24.de>
Signed-off-by: Mike Lloyd <mlloyd@rts.com>
@siennathesane
Copy link
Collaborator Author

@janisz can you take a look at the build problems for me? My knowledge of go modules is weak and I can't figure out the problem.

go.mod Outdated

go 1.12

require github.com/allegro/bigcache/v3 v3.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not needed

go.sum Outdated
@@ -0,0 +1,3 @@
github.com/allegro/bigcache v1.2.1 h1:hg1sY1raCwic3Vnsvje6TT7/pnZba83LeFck5NrFKSc=
github.com/allegro/bigcache/v2 v2.2.5 h1:mRc8r6GQjuJsmSKQNPsR5jQVXc8IJ1xsW5YXUYMLfqI=
github.com/allegro/bigcache/v2 v2.2.5/go.mod h1:FppZsIO+IZk7gCuj5FiIDHGygD9xvWQcqg1uIPMb6tY=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not needed

@janisz
Copy link
Collaborator

janisz commented Nov 4, 2020

#252

diff --git a/examples_test.go b/examples_test.go
index f447072..ccc513a 100644
--- a/examples_test.go
+++ b/examples_test.go
@@ -5,7 +5,7 @@ import (
        "log"
        "time"
 
-       "github.com/allegro/bigcache/v2"
+       "github.com/allegro/bigcache/v3"
 )
 
 func Example() {
diff --git a/go.mod b/go.mod
index 6f045f2..8be47fa 100644
--- a/go.mod
+++ b/go.mod
@@ -1,5 +1,3 @@
 module github.com/allegro/bigcache/v3
 
 go 1.12
-
-require github.com/allegro/bigcache/v2 v2.2.5
diff --git a/go.sum b/go.sum
index fdc2c46..e69de29 100644
--- a/go.sum
+++ b/go.sum
@@ -1,3 +0,0 @@
-github.com/allegro/bigcache v1.2.1 h1:hg1sY1raCwic3Vnsvje6TT7/pnZba83LeFck5NrFKSc=
-github.com/allegro/bigcache/v2 v2.2.5 h1:mRc8r6GQjuJsmSKQNPsR5jQVXc8IJ1xsW5YXUYMLfqI=
-github.com/allegro/bigcache/v2 v2.2.5/go.mod h1:FppZsIO+IZk7gCuj5FiIDHGygD9xvWQcqg1uIPMb6tY=

@janisz janisz mentioned this pull request Nov 4, 2020
@janisz
Copy link
Collaborator

janisz commented Nov 5, 2020

It looks like we have a flaky test TestWriteAndReadParallelSameKeyWithStats

@codecov-io
Copy link

Codecov Report

Merging #245 (7d00ccd) into master (92a824f) will increase coverage by 0.64%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   86.56%   87.20%   +0.64%     
==========================================
  Files          15       15              
  Lines         640      641       +1     
==========================================
+ Hits          554      559       +5     
+ Misses         71       68       -3     
+ Partials       15       14       -1     
Impacted Files Coverage Δ
server/server.go 29.03% <ø> (ø)
queue/bytes_queue.go 88.99% <100.00%> (+0.10%) ⬆️
shard.go 86.75% <100.00%> (+1.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92a824f...7d00ccd. Read the comment docs.

@siennathesane
Copy link
Collaborator Author

@cristaloleg and @janisz I'm working on the interface definition for #204 via 34853d0, do we want to cut 3.0 with just the interface then add in a new default implementation in 3.1? I want to get the interface definition with 3.0, but I don't want the release to be held back on the initial implementation since this fixes #148.

@janisz
Copy link
Collaborator

janisz commented Nov 10, 2020

IMO we should release this and add new features in feature release.

Still I'm not sure if we lost backward compatibility. Do we really need 3.0 release?

@siennathesane
Copy link
Collaborator Author

I'm not sure if we lost backward compatibility

I'm not either but better to be safe than sorry.

@siennathesane siennathesane marked this pull request as ready for review November 11, 2020 01:45
* Go defaults to "0"" so in case we want to return
EntryStatus back to the caller "Expired" cannot be differentiated.
Fixing this by default "_" to 0 and incremental RemoveReasons

comment was missing for GetWithInfo api so updated that

test: ran all unit test cases

* Entry RemoveReason is iota based with default value being 0

Currently all const for RemovedReason are explicitly
set to avoid any breaking changes. For v3 migrating the reasons
to iota makes code readability and managing addition of more reasons
easier in the future.

Test: Validated all test cases run successfully.

Co-authored-by: Mike Lloyd <mike@reboot3times.org>
Co-authored-by: jgheewala <jgheewala@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants