Skip to content
This repository has been archived by the owner on Dec 27, 2020. It is now read-only.

Consumer cached proxy implementation #41

Merged
merged 3 commits into from
Dec 13, 2019

Conversation

alldroll
Copy link
Contributor

@alldroll alldroll commented Dec 9, 2019

It resolves #30

@magicoder10 magicoder10 self-requested a review December 10, 2019 06:53
@magicoder10 magicoder10 added the enhancement New feature or request label Dec 10, 2019
app/app.go Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_cached.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_cached.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_cached.go Outdated Show resolved Hide resolved
dep/provider/consumer.go Outdated Show resolved Hide resolved
dep/provider/consumer.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_test.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_test.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_test.go Outdated Show resolved Hide resolved
@magicoder10
Copy link
Member

Great job on this PR! I love this feature and implementation. Please address the comments.

app/usecase/keys/consumer_test.go Outdated Show resolved Hide resolved
app/usecase/keys/consumer_test.go Outdated Show resolved Hide resolved
app/usecase/repo/repotest/availablekey.go Show resolved Hide resolved
@magicoder10
Copy link
Member

One last change. Impressive work! It's my honor to work with you!

* Unit-testing FIRST principal
* Fixed bug with loadKeys for maxCount > bufferSize
@codecov-io
Copy link

codecov-io commented Dec 12, 2019

Codecov Report

Merging #41 into master will increase coverage by 25.64%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #41       +/-   ##
==========================================
+ Coverage   66.66%   92.3%   +25.64%     
==========================================
  Files           4       5        +1     
  Lines          69     104       +35     
==========================================
+ Hits           46      96       +50     
+ Misses         23       5       -18     
- Partials        0       3        +3
Impacted Files Coverage Δ
app/usecase/keys/consumer_cached.go 94.28% <94.28%> (ø)
app/usecase/keys/consumer.go 80.95% <0%> (+80.95%) ⬆️

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 d9016af...adcf18d. Read the comment docs.

for ; maxCount > 0; maxCount-- {
// there is probability to get the list of keys with the size less than bufferSize
// in this case we listen to done channel to perform loop break
done := make(chan bool)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
done := make(chan bool)
finishRefill := make(chan bool)

if len(p.buffer) == 0 {
go func() {
p.loadKeys()
done <- true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
done <- true
finishRefill <- true

}

keys = append(keys, entry.key)
case <-done:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case <-done:
case <-finishRefill:

app/usecase/keys/consumer_cached.go Show resolved Hide resolved
app/usecase/keys/consumer_cached.go Show resolved Hide resolved
@magicoder10 magicoder10 merged commit 24d5945 into short-d:master Dec 13, 2019
@magicoder10
Copy link
Member

#42

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache keys at server side
3 participants