Skip to content

/ bigcache

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

Jump to bottom

Go defaults to "0"" so in case we want to return #181

Open
wants to merge 7 commits into
base: master
from
Open

Go defaults to "0"" so in case we want to return #181

wants to merge 7 commits into from

Conversation

@jgheewala
Copy link
Contributor

@jgheewala jgheewala commented Oct 9, 2019

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
jgheewala added 2 commits Oct 9, 2019
@jgheewala
Go defaults to "0"" so in case we want to return
91361a0 
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
@jgheewala
Merge branch 'master' into expired_reason_iota_fix
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
02f28af
@jgheewala jgheewala mentioned this pull request Oct 9, 2019
Closed
@cristaloleg cristaloleg added this to the 3.0.0 milestone Oct 9, 2019
@mxplusb
Copy link
Collaborator

@mxplusb mxplusb commented Oct 9, 2019

I quite like this PR and problem it solves, but I agree this is definitely a 3.0 decision since it could break a lot of downstream configurations when they upgrade. This should definitely go in the release notes.
@jgheewala
Copy link
Contributor Author

@jgheewala jgheewala commented Oct 9, 2019

@mxplusb this PR can be open until its ready to go in. I kept it here so that I do not forget about this :)
@mxplusb mxplusb added the future label Oct 9, 2019
@mxplusb
Copy link
Collaborator

@mxplusb mxplusb commented Oct 9, 2019

Great, thanks!
@flisky
Copy link
Contributor

@flisky flisky commented Mar 2, 2020

So, the error ErrEntryIsDead has no more referenced and should be removed ? Will give users a compile error in case they don't notice this breaking change.
@cristaloleg
Copy link
Collaborator

@cristaloleg cristaloleg commented Mar 2, 2020

@flisky good point, we can remove it. But anyway, this PR is for a next major version, so it will be safe to make a breaking change.
@mxplusb
Copy link
Collaborator

@mxplusb mxplusb commented Mar 3, 2020

@jgheewala when you get a chance, can you rebase? It would be good to keep this updated to prep for the next major version.
@flisky
Copy link
Contributor

@flisky flisky commented Mar 25, 2020

May I take over of it, @jgheewala & @mxplusb ?

Besides these changes, I also want to return the expired entry even when resp.EntryStatus = Expired( entry goes []byte when introduced in #168 , and then goes to nil in #191 ). Do you like the idea?
@cristaloleg
Copy link
Collaborator

@cristaloleg cristaloleg commented Mar 25, 2020

@flisky let's create a separate issue for your idea and discuss it there. This will make discussion much more visible and structured, thanks :)
@flisky flisky mentioned this pull request Apr 1, 2020
Merged
@jgheewala
Copy link
Contributor Author

@jgheewala jgheewala commented Apr 15, 2020

@mxplusb was super busy will rebase my changes soon
@jgheewala
Merge branch 'master' into expired_reason_iota_fix
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1b84d1c
@jgheewala
Copy link
Contributor Author

@jgheewala jgheewala commented Aug 25, 2020

@mxplusb done with the conflicts.
@mxplusb
Merge branch 'master' into expired_reason_iota_fix
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8eb2dac
@mxplusb mxplusb mentioned this pull request Aug 31, 2020
Open
@jgheewala
Merge branch 'master' into expired_reason_iota_fix
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e2ef8ae
@mxplusb
Copy link
Collaborator

@mxplusb mxplusb commented Oct 5, 2020

@jgheewala there's a new branch, called versions/v3-prep which has the underlying necessary module changes to support this PR. If you can rebase against that branch, and then edit the PR to point to the versions/v3-prep branch, I can get this merged and we can close it out.
@jgheewala
Merge branch 'master' into expired_reason_iota_fix
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
83cd9cd
@codecov-io
Copy link

@codecov-io codecov-io commented Nov 17, 2020 •
edited

Codecov Report

Merging #181 (c3845a8) into master (92a824f) will increase coverage by 0.45%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   86.56%   87.01%   +0.45%     
==========================================
  Files          15       15              
  Lines         640      647       +7     
==========================================
+ Hits          554      563       +9     
+ Misses         71       68       -3     
- Partials       15       16       +1
Impacted Files Coverage Δ
bigcache.go 98.79% <ø> (ø)
queue/bytes_queue.go 87.82% <0.00%> (-1.07%) ⬇️
shard.go 86.75% <0.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...c3845a8. Read the comment docs.
@jgheewala jgheewala mentioned this pull request Nov 17, 2020
Open
@jgheewala
Copy link
Contributor Author

@jgheewala jgheewala commented Nov 17, 2020

@mxplusb #255
jgheewala
Entry RemoveReason is iota based with default value being 0
Loading status checks…
c3845a8 
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.
@janisz
janisz approved these changes Nov 18, 2020
View changes
@cristaloleg
cristaloleg approved these changes Nov 19, 2020
View changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristaloleg cristaloleg

@janisz janisz

Assignees
No one assigned
Labels
discussion enhancement future
Projects
None yet
Milestone
3.0.0
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@jgheewala @mxplusb @flisky @cristaloleg @codecov-io @janisz
You can’t perform that action at this time.