refactor storage implementation #243

zufardhiyaulhaq · 2021-04-11T05:22:17Z

refactor storage implementation to use design pattern. this is also a step to implement rolling window support in #193

currently there is no unit test, let me know if this is make sense and I can write the unit test.

Signed-off-by: zufardhiyaulhaq [email protected]

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq · 2021-04-11T05:24:35Z

@mattklein123 @nezdolik needs your help to review. I move all implementation details related to storage to StorageStrategy.

nezdolik · 2021-04-16T16:06:16Z

Thanks @zufardhiyaulhaq, i will review shortly.

nezdolik

@zufardhiyaulhaq i've done first pass, will need to do one more. Please add unit/integration tests for this change.

nezdolik · 2021-04-19T21:50:03Z

src/storage/strategy/storage_strategy.go

+package strategy
+
+type StorageStrategy interface {
+	GetValue(key string) (uint64, error)


nit: add docs to interface methods

ping on this

nezdolik · 2021-04-19T21:53:46Z

src/redis/fixed_cache_impl.go

 	// Now fetch the pipeline.
 	responseDescriptorStatuses := make([]*pb.RateLimitResponse_DescriptorStatus,
 		len(request.Descriptors))
 	for i, cacheKey := range cacheKeys {

-		limitAfterIncrease := results[i]
-		limitBeforeIncrease := limitAfterIncrease - hitsAddend
+		limitBeforeIncrease := uint32(results[i])


is this change intended? eg limitBeforeIncrease used to be results[i] - hitsAddend and with this change it will be uint32(results[i])

yes this is intended. since we are not increasing in this function, so it's should be the result that we get from redis. We will increase the value in increaseAsync function below.

github-actions · 2021-05-21T20:27:05Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

nezdolik

@zufardhiyaulhaq PR mostly looks good to me (apart from missing tests). It's a large PR to review, but big part of it just moving code around, so functionality should remain the same.

nezdolik · 2021-05-31T09:56:25Z

src/redis/cache_impl.go

 			s.RedisPerSecondType, s.RedisPerSecondUrl, s.RedisPerSecondPoolSize, s.RedisPerSecondPipelineWindow, s.RedisPerSecondPipelineLimit)
 	}
-	var otherPool Client
-	otherPool = NewClientImpl(srv.Scope().Scope("redis_pool"), s.RedisTls, s.RedisAuth, s.RedisType, s.RedisUrl, s.RedisPoolSize,
+	var otherPool storage_strategy.StorageStrategy


nit: use inline initialisation here

nezdolik · 2021-05-31T10:46:39Z

src/storage/service/redis_client.go

+	"github.com/mediocregopher/radix/v3"
+)
+
+type RedisClientInterface interface {


nit: add docs to interface methods

same here, comment repeats type name

nezdolik · 2021-05-31T10:46:44Z

src/storage/service/memcached_client.go

+	"github.com/bradfitz/gomemcache/memcache"
+)
+
+type MemcachedClientInterface interface {


nit: add docs to interface methods

nit: this comment looks rudimentary, as it repeats the name of type, either add more explanatory comment or remove it.

nezdolik · 2021-05-31T10:48:32Z

test/memcached/cache_impl_test.go

-// simply by incrementing it but memcached cannot. In memcache new keys need to be explicitly
-// added.
-package memcached_test
-


please add tests for refactored code

Hi sorry for very slow resp, will start creating unit test for the code

Signed-off-by: zufardhiyaulhaq <[email protected]>

zufardhiyaulhaq · 2021-06-07T16:47:14Z

@nezdolik need your help to review, rebase from master and add the unit test.

nezdolik · 2021-06-24T19:29:18Z

@zufardhiyaulhaq somehow i missed your comment, i will review by start of next week.

nezdolik · 2021-07-02T10:08:50Z

src/redis/fixed_cache_impl.go

+		if this.perSecondClient != nil && cacheKey.PerSecond {
+			err := this.perSecondClient.IncrementValue(cacheKey.Key, uint64(hitsAddend))
+			if err != nil {
+				logger.Error(err)
+			}
+
+			err = this.perSecondClient.SetExpire(cacheKey.Key, uint64(expirationSeconds))
+			if err != nil {
+				logger.Error(err)
+			}
+		} else {
+			err := this.client.IncrementValue(cacheKey.Key, uint64(hitsAddend))
+			if err != nil {
+				logger.Error(err)
+			}
+
+			err = this.client.SetExpire(cacheKey.Key, uint64(expirationSeconds))
+			if err != nil {
+				logger.Error(err)
+			}
+		}
 	}


this section looks like duplicated code

yeah but it's not, it's using different client to execute

perhaps you could extract it to a function that accepts a client as argument?

nezdolik · 2021-07-02T10:14:46Z

src/redis/fixed_cache_impl.go

+		client:                     client,
+		perSecondClient:            perSecondClient,
+		jitterRand:                 jitterRand,
+		expirationJitterMaxSeconds: expirationJitterMaxSeconds,


so here we pass jitter settings both in base rate limiter and in fixedRateLimitCacheImpl, should stick to one class instead

nezdolik · 2021-07-02T10:20:48Z

src/storage/factory/memcached_factory.go

+	if srv != "" {
+		client = newMemcachedClientFromSrv(scope, srv, srvRefresh, maxIdleConnection)
+	} else {
+


either remove empty block or put client initialization with hosts here

nezdolik · 2021-07-02T10:28:56Z

src/storage/factory/memcached_factory.go

+	defer t.Stop()
+	for {
+		select {
+		case <-t.C:


not related to PR itself: i know this is Go convention, but is hard to read (if one is not familiar with Tickers in Go)

nezdolik · 2021-07-02T10:33:22Z

src/storage/service/memcached_client.go

+	"github.com/bradfitz/gomemcache/memcache"
+)
+
+type MemcachedClientInterface interface {


nit: this comment looks rudimentary, as it repeats the name of type, either add more explanatory comment or remove it.

nezdolik

This is great refactoring, thank you. Added comments for bunch of nits, but overall makes sense to me. Would the next step be plugging different algorithms into storage strategy?

nezdolik · 2021-07-02T12:42:57Z

src/storage/service/redis_client.go

+	"github.com/mediocregopher/radix/v3"
+)
+
+type RedisClientInterface interface {


same here, comment repeats type name

nezdolik · 2021-07-02T13:06:34Z

src/storage/strategy/memcached_strategy.go

+	"github.com/envoyproxy/ratelimit/src/storage/service"
+)
+
+type MemcachedStrategy struct {


nit: use compile time check if given type implements interface, example here: https://github.com/envoyproxy/ratelimit/blob/main/src/memcached/cache_impl.go#L55

nezdolik · 2021-07-02T13:06:48Z

src/storage/strategy/redis_strategy.go

+	"github.com/mediocregopher/radix/v3"
+)
+
+type RedisStrategy struct {


nit: use compile time check if given type implements interface, example here: https://github.com/envoyproxy/ratelimit/blob/main/src/memcached/cache_impl.go#L55

nezdolik · 2021-07-02T13:07:08Z

src/storage/strategy/storage_strategy.go

+package strategy
+
+type StorageStrategy interface {
+	GetValue(key string) (uint64, error)


ping on this

nezdolik · 2021-07-02T13:07:17Z

src/storage/strategy/storage_strategy.go

+package strategy
+
+// Interface to abstract underlying storage like memcached and redis
+// Implement bussiness level where we don't care how underlying storage doing it.\


extra slash in the end?

github-actions · 2021-08-01T16:06:29Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-08-08T16:07:24Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

refactor storage impelementation

c45c62e

Signed-off-by: zufardhiyaulhaq <[email protected]>

mattklein123 assigned nezdolik Apr 12, 2021

nezdolik reviewed Apr 19, 2021

View reviewed changes

github-actions bot added the stale label May 21, 2021

mattklein123 self-assigned this May 25, 2021

nezdolik reviewed May 31, 2021

View reviewed changes

zufardhiyaulhaq added 4 commits May 31, 2021 15:37

add storage factory test

db46531

Signed-off-by: zufardhiyaulhaq <[email protected]>

add storage strategy unit test

c50480a

Signed-off-by: zufardhiyaulhaq <[email protected]>

implement storage stats

9ae450c

Signed-off-by: zufardhiyaulhaq <[email protected]>

add fixed redis cache implementation test

04e8192

Signed-off-by: zufardhiyaulhaq <[email protected]>

mattklein123 added the waiting label Jun 1, 2021

add fixed memcached cache implementation test

9f01384

Signed-off-by: zufardhiyaulhaq <[email protected]>

repokitteh-read-only bot removed the waiting label Jun 5, 2021

zufardhiyaulhaq added 3 commits June 5, 2021 11:35

Merge https://github.com/envoyproxy/ratelimit into refactor-storage

343d5ff

Signed-off-by: zufardhiyaulhaq <[email protected]>

fix format and memcached test

2f7bd25

Signed-off-by: zufardhiyaulhaq <[email protected]>

remove unused code in fixed memcached cache implementation

bfa70cf

Signed-off-by: zufardhiyaulhaq <[email protected]>

github-actions bot removed the stale label Jun 7, 2021

mattklein123 added the waiting:any label Jun 11, 2021

repokitteh-read-only bot removed the waiting:any label Jun 24, 2021

mattklein123 added the waiting label Jun 28, 2021

nezdolik reviewed Jul 2, 2021

View reviewed changes

github-actions bot added the stale label Aug 1, 2021

github-actions bot closed this Aug 8, 2021

ysawa0 mentioned this pull request Dec 10, 2021

Rolling window limits? #32

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor storage implementation #243

refactor storage implementation #243

zufardhiyaulhaq commented Apr 11, 2021

zufardhiyaulhaq commented Apr 11, 2021

nezdolik commented Apr 16, 2021

nezdolik left a comment

nezdolik Apr 19, 2021

nezdolik Jul 2, 2021

nezdolik Apr 19, 2021

zufardhiyaulhaq Apr 21, 2021

github-actions bot commented May 21, 2021

nezdolik left a comment

nezdolik May 31, 2021

nezdolik May 31, 2021

nezdolik Jul 2, 2021

nezdolik May 31, 2021

nezdolik Jul 2, 2021

nezdolik May 31, 2021

zufardhiyaulhaq May 31, 2021

zufardhiyaulhaq commented Jun 7, 2021

nezdolik commented Jun 24, 2021

nezdolik Jul 2, 2021

zufardhiyaulhaq Jul 2, 2021

nezdolik Jul 2, 2021 •

edited

Loading

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik left a comment

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

nezdolik Jul 2, 2021

github-actions bot commented Aug 1, 2021

github-actions bot commented Aug 8, 2021

refactor storage implementation #243

refactor storage implementation #243

Conversation

zufardhiyaulhaq commented Apr 11, 2021

zufardhiyaulhaq commented Apr 11, 2021

nezdolik commented Apr 16, 2021

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 21, 2021

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zufardhiyaulhaq commented Jun 7, 2021

nezdolik commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezdolik Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezdolik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 1, 2021

github-actions bot commented Aug 8, 2021

nezdolik Jul 2, 2021 •

edited

Loading