Couple of changes to exercise 2 #12

benprew · 2020-08-14T04:57:54Z

Hey,

Thanks for putting these exercise together, they're fun!

I wanted to expand on the exercise a little bit by adding some more detail to the problem and also adding an artificial slowdown to the mock database call to better simulate a call to a database. It also highlights the effects of different solutions (ie locking the entire function vs locking just the map/list access)

Finally I was noticed in the comments that it mentioned that it was an LRU cache, but it's implemented as a FIFO cache, so I included some changes to implement an LRU cache.

Let me know if there's any changes you'd like me to make, I'm happy to make any fixes.

Thanks!

This additional slowdown offers the change to optimizing the locking code.

loong · 2020-08-21T07:31:35Z

Hey Ben,

Thank you so much for the PR! Looks great so far. Will give it a closer view over the weekend! 🙏

Greetings from Singapore,
Long

benprew · 2020-08-22T04:39:54Z

Long, No problem, thanks for providing the exercises!

…

On Fri, Aug 21, 2020 at 12:31 AM Long Hoang ***@***.***> wrote: Hey Ben, Thank you so much for the PR! Looks great so far. Will give it a closer view over the weekend! 🙏 Greetings from Singapore, Long — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJHV5LIW367BYSVPUFD5DSBYPGJANCNFSM4P7D3L2Q> .

loong

Hey @benprew!

Thank you so much for the PR! Especially catching my mistake with the LRU. I hope no one used my implementation as a template for their own LRU cache implementation 😅.

I ran golint and got some errors. Better to solve them or my friend @ironzeb will give us a bad GoReportCard!

~/g/s/g/l/g/2-race-in-cache ❯❯❯ golint .
main.go:22:6: exported type Page should have comment or be unexported
main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

So, I only have a these small nitpicks. Once those are resolved, we can merge!

Greetings from tropical Singapore!

loong · 2020-08-24T12:56:25Z

2-race-in-cache/check_test.go

+	cache := New(&loader)
+
+	for i := 0; i < 100; i++ {
+		cache.Get("Test " + strconv.Itoa(i))


Suggested change

cache.Get("Test " + strconv.Itoa(i))

cache.Get("Test" + strconv.Itoa(i))

I'm really nitpicking here, sorry 🙇 ! So, in the mock server, there isn't a space after Test:
https://github.com/loong/go-concurrency-exercises/blob/master/2-race-in-cache/mockserver.go#L29

It's all correct though 👍

loong · 2020-08-24T12:57:10Z

2-race-in-cache/check_test.go

@@ -20,3 +23,24 @@ func TestMain(t *testing.T) {
 		t.Errorf("Incorrect pages size %v", pagesLen)
 	}
 }
+
+func TestLRU(t *testing.T) {


Test case! 🤩

loong · 2020-08-24T13:03:02Z

2-race-in-cache/main.go

@@ -19,9 +19,14 @@ type KeyStoreCacheLoader interface {
 	Load(string) string
 }

+type Page struct {


Page is an exported value but has no comment!

From linter:
main.go:22:6: exported type Page should have comment or be unexported

loong · 2020-08-24T13:04:18Z

2-race-in-cache/main.go

+	if e, ok := k.cache[key]; ok {
+		k.pages.MoveToFront(e)
+		return e.Value.(Page).Value
+	} else {
+		// Miss - load from database and save it in cache
+		page := Page{key, k.load(key)}
 		// if cache is full remove the least used item
-		if len(k.cache) > CacheSize {
-			delete(k.cache, k.pages.Back().Value.(string))
-			k.pages.Remove(k.pages.Back())
+		if len(k.cache) >= CacheSize {
+			end := k.pages.Back()
+			// remove from map
+			delete(k.cache, end.Value.(Page).Key)
+			// remove from list
+			k.pages.Remove(end)
 		}
+		k.pages.PushFront(page)
+		k.cache[key] = k.pages.Front()
+		return page.Value


The if block ends with a return, so else block is not needed.

From linter:
main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

benprew · 2020-08-28T00:58:21Z

Great, thanks for the feedback, I'll make those changes and update the PR.

…

On Mon, Aug 24, 2020 at 6:07 AM Long Hoang ***@***.***> wrote: ***@***.**** requested changes on this pull request. Hey @benprew <https://github.com/benprew>! Thank you so much for the PR! Especially catching my mistake with the LRU. I hope no one used my implementation as a template for their own LRU cache implementation 😅. I ran golint and got some errors. Better to solve them or my friend @ironzeb will give us a bad GoReportCard <https://goreportcard.com/report/github.com/loong/go-concurrency-exercises> ! ~/g/s/g/l/g/2-race-in-cache ❯❯❯ golint . main.go:22:6: exported type Page should have comment or be unexported main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) So, I only have a these small nitpicks. Once those are resolved, we can merge! Greetings from tropical Singapore! ------------------------------ In 2-race-in-cache/check_test.go <#12 (comment)> : > @@ -20,3 +23,24 @@ func TestMain(t *testing.T) { t.Errorf("Incorrect pages size %v", pagesLen) } } + +func TestLRU(t *testing.T) { + loader := Loader{ + DB: GetMockDB(), + } + cache := New(&loader) + + for i := 0; i < 100; i++ { + cache.Get("Test " + strconv.Itoa(i)) ⬇️ Suggested change - cache.Get("Test " + strconv.Itoa(i)) + cache.Get("Test" + strconv.Itoa(i)) I'm really nitpicking here, sorry 🙇 ! So, in the mock server, there isn't a space after Test: https://github.com/loong/go-concurrency-exercises/blob/master/2-race-in-cache/mockserver.go#L29 It's all correct though 👍 ------------------------------ In 2-race-in-cache/check_test.go <#12 (comment)> : > @@ -20,3 +23,24 @@ func TestMain(t *testing.T) { t.Errorf("Incorrect pages size %v", pagesLen) } } + +func TestLRU(t *testing.T) { Test case! 🤩 ------------------------------ In 2-race-in-cache/main.go <#12 (comment)> : > @@ -19,9 +19,14 @@ type KeyStoreCacheLoader interface { Load(string) string } +type Page struct { Page is an exported value but has no comment! From linter: main.go:22:6: exported type Page should have comment or be unexported ------------------------------ In 2-race-in-cache/main.go <#12 (comment)> : > + if e, ok := k.cache[key]; ok { + k.pages.MoveToFront(e) + return e.Value.(Page).Value + } else { + // Miss - load from database and save it in cache + page := Page{key, k.load(key)} // if cache is full remove the least used item - if len(k.cache) > CacheSize { - delete(k.cache, k.pages.Back().Value.(string)) - k.pages.Remove(k.pages.Back()) + if len(k.cache) >= CacheSize { + end := k.pages.Back() + // remove from map + delete(k.cache, end.Value.(Page).Key) + // remove from list + k.pages.Remove(end) } + k.pages.PushFront(page) + k.cache[key] = k.pages.Front() + return page.Value The if block ends with a return, so else block is not needed. From linter: main.go:47:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAJHV22N6OZZ274NZC7K5TSCJQ2LANCNFSM4P7D3L2Q> .

benprew · 2020-08-29T21:34:38Z

Hey @loong thanks for the feedback, I think my changes address your feedback and the golint warnings. Let me know if I missed anything.

How's the weather in Singapore? I'm in Oregon, USA, where it's currently 70F (21C) and sunny, pretty perfect out right now.

loong · 2020-08-30T12:57:59Z

Well, it's an eternal summer here with at least 30°C (86°F) during the day. I grew up in Germany, so this is hot for me (especially with the humidity!). 😅

Went for a hike today!

EDIT: The picture above is totally necessary for this PR

loong

LGTM! Feel free to merge 🎉

Thank you so much for your contribution! Also, feel free to add your own exercise!

loong · 2020-08-30T12:53:30Z

2-race-in-cache/main.go

@@ -19,7 +19,7 @@ type KeyStoreCacheLoader interface {
 	Load(string) string
 }

-type Page struct {
+type page struct {


Haha, well that's also a way to pass the linter 😂

benprew · 2020-09-06T18:56:49Z

Thanks @loong, I'll send another PR if I think of some exercises 👍 . Also, I don't think I can merge this PR unless you grant me write access to your repo.

PS. that's a great picture, it's so beautiful and serene, more PRs could use pictures like that :)

benprew added 4 commits August 13, 2020 20:28

Fix bug with LRU implementation

2d3d847

Add a little delay to the database call.

53b9b7b

This additional slowdown offers the change to optimizing the locking code.

Add additional detail on the problem and some examples of caches in Go

706bdd3

Fix README formatting

9231805

loong requested changes Aug 24, 2020

View reviewed changes

fix golint errors and requested changes in PR

cd4ebd6

loong approved these changes Aug 30, 2020

View reviewed changes

loong merged commit a36886f into loong:master Sep 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Couple of changes to exercise 2 #12

Couple of changes to exercise 2 #12

benprew commented Aug 14, 2020

loong commented Aug 21, 2020

benprew commented Aug 22, 2020 via email

loong left a comment

loong Aug 24, 2020

loong Aug 24, 2020

loong Aug 24, 2020

loong Aug 24, 2020

benprew commented Aug 28, 2020 via email

benprew commented Aug 29, 2020

loong commented Aug 30, 2020 •

edited

Loading

loong left a comment

loong Aug 30, 2020

benprew commented Sep 6, 2020

	cache.Get("Test " + strconv.Itoa(i))
	cache.Get("Test" + strconv.Itoa(i))

Couple of changes to exercise 2 #12

Couple of changes to exercise 2 #12

Conversation

benprew commented Aug 14, 2020

loong commented Aug 21, 2020

benprew commented Aug 22, 2020 via email

loong left a comment

Choose a reason for hiding this comment

loong Aug 24, 2020

Choose a reason for hiding this comment

loong Aug 24, 2020

Choose a reason for hiding this comment

loong Aug 24, 2020

Choose a reason for hiding this comment

loong Aug 24, 2020

Choose a reason for hiding this comment

benprew commented Aug 28, 2020 via email

benprew commented Aug 29, 2020

loong commented Aug 30, 2020 • edited Loading

loong left a comment

Choose a reason for hiding this comment

loong Aug 30, 2020

Choose a reason for hiding this comment

benprew commented Sep 6, 2020

loong commented Aug 30, 2020 •

edited

Loading