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

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

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

Conversation

jgheewala
Copy link
Contributor

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

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
@cristaloleg cristaloleg added this to the 3.0.0 milestone Oct 9, 2019
@siennathesane
Copy link
Collaborator

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

@mxplusb this PR can be open until its ready to go in. I kept it here so that I do not forget about this :)

@siennathesane
Copy link
Collaborator

Great, thanks!

@flisky
Copy link
Contributor

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

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

@siennathesane
Copy link
Collaborator

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

@flisky let's create a separate issue for your idea and discuss it there. This will make discussion much more visible and structured, thanks :)

@jgheewala
Copy link
Contributor Author

@mxplusb was super busy will rebase my changes soon

@jgheewala
Copy link
Contributor Author

@mxplusb done with the conflicts.

@siennathesane siennathesane mentioned this pull request Aug 31, 2020
@siennathesane
Copy link
Collaborator

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

@codecov-io
Copy link

codecov-io commented Nov 17, 2020

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
Copy link
Contributor Author

@mxplusb #255

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.
@codecov-commenter
Copy link

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.

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

7 participants