Upgrade radix #137

caitong93 · 2020-05-20T08:02:24Z

issue: #129

Changes:

Replace Pool with Client, as in radix Client is abstract interface for pool, sentinel and redis cluster.
Refactor redis cache impl to use implicit pipelining to improve throughput.

cc @mattklein123

mattklein123

Nice this is great. Looks really good at a high level.

Do any documentation need to be updated about supported config variables on the README? Can you check?

@junr03 @freedomljc can you both take a look? Thank you!

mattklein123 · 2020-05-20T22:37:22Z

src/redis/cache_impl.go

 	timeSource                 TimeSource
 	jitterRand                 *rand.Rand
 	expirationJitterMaxSeconds int64
-	// bytes.Buffer pool used to efficiently generate cache keys.
+	// bytes.Buffer client used to efficiently generate cache keys.


I think this is an errant find/replace

mattklein123 · 2020-05-20T22:40:46Z

src/redis/driver_impl.go

+	stats := newPoolStats(scope)
+
+	// TODO: support sentinel and redis cluster
+	pool, err := radix.NewPool("tcp", url, poolSize, radix.PoolConnFunc(df),


cc @moderation I think there was some regression here where we no longer support unix domain sockets, is that right? If so can we fix that also either here or in a follow up?

Here is the issue for the socket regression where the functionality was completed removed in a prior commit - #133. I unblocked myself but I think reintroduction will require a bit more work. Tests lacking if it was possible to delete the default documented working mode

caitong93 · 2020-05-21T12:45:28Z

Do any documentation need to be updated about supported config variables on the README? Can you check?

description about new config variables have been updated in README

caitong93 · 2020-05-21T13:05:31Z

src/settings/settings.go

+	RedisPoolSize                int           `envconfig:"REDIS_POOL_SIZE" default:"10"`
+	RedisAuth                    string        `envconfig:"REDIS_AUTH" default:""`
+	RedisTls                     bool          `envconfig:"REDIS_TLS" default:"false"`
+	RedisPipelineWindow          time.Duration `envconfig:"REDIS_PIPELINE_WINDOW" default:"75µs"`


Defaults for pipeline window and limit are chosen according to my e2e benchmark results. Also have a look at this issue.

I set up a 8c8g envoy with 10 2c2g ratelimit servers, and got 60000 requests/second . 75us window seems better than 150us(radix default) in this load.

freedomljc

Some small comments, but good work to make pipeline logic better.

freedomljc · 2020-05-21T16:03:28Z

src/redis/cache_impl.go

-	// limits regardless of unit. If this pool is not nil, then it
+	client Client
+	// Optional Client for a dedicated cache of per second limits.
+	// If this client is nil, then the Cache will use the client for all


Might be not related to your change.
But the description seems not right regarding if this client is nil. I think we need to specify it as at least a pointer, then we can determine whether it is nil or not.

It is an interface, should be OK for nil testing.

freedomljc · 2020-05-21T16:07:14Z

src/redis/cache_impl.go

@@ -154,6 +141,8 @@ func (this *rateLimitCacheImpl) DoLimit(
 	}

 	isOverLimitWithLocalCache := make([]bool, len(request.Descriptors))
+	results := make([]uint32, len(request.Descriptors), len(request.Descriptors))


Is it intended? can you let us know why we need to specify make([]uint32, len(request.Descriptors), len(request.Descriptors)) instead of make([]uint32, len(request.Descriptors))

No, it's not intended. Will remove the redundant.

freedomljc · 2020-05-21T16:11:04Z

src/redis/cache_impl.go

@@ -178,13 +167,20 @@ func (this *rateLimitCacheImpl) DoLimit(
 			expirationSeconds += this.jitterRand.Int63n(this.expirationJitterMaxSeconds)
 		}

+		result := &results[i]
+		key := cacheKey.key


I'd suggest not to add new variables result and key here, as the existing variables results[i] and cacheKey.key are already obvious to understand.

freedomljc · 2020-05-21T16:33:40Z

src/redis/cache_impl.go

 		}
 	}
+	checkError(lastErr)


can you let us know why only to panic in last error, instead of panic in the first error?

When wrote this code, I was thinking to execute all commands and recording all errors. For simplicity, only lastErr was checked. But now thinking again, it seems panic in the first error is enough. I will update this latter.

freedomljc · 2020-05-21T16:42:32Z

src/redis/driver_impl.go

-			conn, err = tls.Dial("tcp", addr, &tls.Config{})
-		} else {
-			conn, err = net.Dial("tcp", addr)
+			dialOpts = append(dialOpts, radix.DialUseTLS(&tls.Config{}))


might be a dumb question. For non-tls, non-auth case, the dialOpts would be empty, is it still able to connect to redis?

freedomljc · 2020-05-21T16:50:52Z

src/redis/driver_impl.go

+	pool, err := radix.NewPool("tcp", url, poolSize, radix.PoolConnFunc(df),
+		radix.PoolPipelineWindow(pipelineWindow, pipelineLimit),
+		radix.PoolWithTrace(poolTrace(&stats)),
+	)
 	checkError(err)


Can you add some test cases for the constructor NewClientImpl ? especially when there exist some errors.

freedomljc · 2020-05-21T16:55:06Z

src/redis/driver_impl.go

-func (this *connectionImpl) PipeResponse() Response {
-	assert.Assert(this.pending > 0)
-	this.pending--
+func (c *clientImpl) DoCmd(rcv interface{}, cmd, key string, args ...interface{}) error {


Same here, it would be great if we have some tests to cover the error cases.

caitong93 · 2020-05-26T13:45:00Z

@freedomljc have fixed comments and added some unit tests(just cover some basic cases).
Can you take a look again when you have time? Thanks :)

freedomljc

Few more comments about unit test and benchmark test.
Regarding the default values, thanks for letting us know how 75us get chosen for pipelineWindow. It would be great if you have more analysis about how 8 get chosen for pipelineLimit as well.

freedomljc · 2020-05-27T00:46:59Z

test/redis/bench_test.go

+	b.Run("pipeline 150us 4", mkDoLimitBench(150*time.Microsecond, 4))
+	b.Run("pipeline 50us 8", mkDoLimitBench(50*time.Microsecond, 8))
+	b.Run("pipeline 75us 8", mkDoLimitBench(75*time.Microsecond, 8))
+	b.Run("pipeline 150us 8", mkDoLimitBench(150*time.Microsecond, 8))


It might be better to add more benchmark tests, and let us know why 8 get chosen here?

I compared 4 , 8 and 16 in e2e benchmark(75us window), using tcpdump to observe actual pipeline size. Found many requests to redis have a pipeline size greater than 4, nearly 8. This analysis is very rough, but 8 should be ok to use in most situations.

freedomljc · 2020-05-27T01:05:57Z

test/redis/cache_impl_test.go

+		assert.Panics(t, func() {
+			// It's possible there is a redis server listening on 6379 in ci environment, so
+			// use a random port.
+			mkRedisClient("", "localhost:12345")


Is it to test the ping/pong connection ? can we also check the error?

It is to test connection refused case. ping/pong is used to check auth.(we don't know if auth is ok util receive first response from redis).
Error check is added.

freedomljc · 2020-05-27T01:16:11Z

test/redis/cache_impl_test.go

+		assert.NotPanics(t, func() {
+			mkRedisClient(redisAuth, redisSrv.Addr())
+		})
+	})


is it possible to test tls case as well?

tls support is not available for now. Maybe we can do it in another pr when
alicebob/miniredis#155 solved ?

okay, sound fair.

…eline Signed-off-by: Tong Cai <caitong93@gmail.com>

Signed-off-by: Tong Cai <caitong93@gmail.com>

caitong93 · 2020-05-27T15:31:41Z

@freedomljc Updated

freedomljc

LGTM, and pls resolve the conflict.
One more suggestion: pls do not force-push the pr, as it loses track of the context among commits/comments.

Signed-off-by: Tong Cai <caitong93@gmail.com>

caitong93 · 2020-05-28T13:43:38Z

One more suggestion: pls do not force-push the pr, as it loses track of the context among commits/comments.

Thanks the suggestion ! ( always forgot DCO and use force-push to fix, will pay attention next time )

Conflicts resolved, PTAL @freedomljc @mattklein123

mattklein123

Thanks!

A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]:

A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: Signed-off-by: David Weitzman <dweitzman@pinterest.com>

A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com>

A newly-added test in #137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com>

Signed-off-by: Tong Cai <caitong93@gmail.com> Signed-off-by: Diego Erdody <diego@medallia.com>

A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com> Signed-off-by: Diego Erdody <diego@medallia.com>

* Add Docker Compose File (#27) * docs: match envoy docs for remote_address ratelimiting (#29) * docs: document dependency on gostats (#30) * test and document whitelist behavior (#31) Signed-off-by: Daniel Hochman <danielhochman@users.noreply.github.com> * update dependencies (#35) * update dependencies * proto: use the proto defined in data-plane-api (#39) * proto: check in protos to allow importing ratelimit as a library (#40) * docs: update contact info (#42) * redis: add the option to use a separate redis pool for per second limits (#41) * fix duplicate mv (#43) * docker: upgrade docker-compose setup (#46) * Add gRPC health check (#47) * logging: set log level (#50) * go version: update to 1.11 (#53) * docker compose: expose gRPC port on docker compose setup (#55) * Configuration to ignore dotfiles. (#52) This allows ratelimit to run on Kubernetes with configuration from a configmap. * Add Dockerfile to enable builds (#58) Signed-off-by: Steve Sloka <steves@heptio.com> * ci: fix build (#73) Fixes envoyproxy#71 Signed-off-by: Matt Klein <mklein@lyft.com> * Run unit and integration tests with race detector enabled (#65) * deps: update several of ratelimit's dependencies (#76) * add stalebot (#78) Signed-off-by: Matt Klein <mklein@lyft.com> * docs: fix example 4 sample config (#79) * fix redis-server binary name (envoyproxy#88) * Fix build Dockerfile (envoyproxy#98) Fix problem: src/service_cmd/runner/runner.go:10:2: cannot find package "github.com/lyft/ratelimit/proto/ratelimit" in any of: /usr/local/go/src/github.com/lyft/ratelimit/vendor/github.com/lyft/ratelimit/proto/ratelimit (vendor tree) /usr/local/go/src/vendor/github.com/lyft/ratelimit/proto/ratelimit /usr/local/go/src/github.com/lyft/ratelimit/proto/ratelimit (from $GOROOT) /go/src/github.com/lyft/ratelimit/proto/ratelimit (from $GOPATH) The command '/bin/sh -c go build -o /usr/local/bin/ratelimit src/service_cmd/main.go' returned a non-zero code: 1 * Redis TLS and Auth support (envoyproxy#96) This adds support for TLS connections to redis as well as support for authentication. Somewhat related to issue #61 * healthcheck: allow customizable healthcheck name (envoyproxy#102) Description: this patch allows a consumer of the server package to customize the name of the healthchecker. Signed-off-by: Jose Nino <jnino@lyft.com> * health: make a few more types public (envoyproxy#104) Description: envoyproxy#102 allowed for some customization. This PR makes the types public so that other servers can use this implementation. Signed-off-by: Jose Nino <jnino@lyft.com> * Add local cache to store whether it is over the limit (envoyproxy#111) * Plugin statstore into runner (envoyproxy#115) * fix: support auth without tls (envoyproxy#116) Signed-off-by: tangxinfa <tangxinfa@gmail.com> * add local cache stats (envoyproxy#114) * Move license to templated Apache-2.0 (envoyproxy#123) Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com> * Enable go modules (envoyproxy#124) Signed-off-by: Steve Sloka <slokas@vmware.com> * CI: Github Actions (envoyproxy#127) Signed-off-by: Steve Sloka <slokas@vmware.com> * community: update contributing guide (envoyproxy#139) Fixes envoyproxy#138 Signed-off-by: Matt Klein <mklein@lyft.com> * add http 1 `/json` endpoint (envoyproxy#136) Signed-off-by: David Black <david.black@autodesk.com> * Use mockgen version from go.mod instead of from "make bootstrap" (envoyproxy#143) Even though the Makefile wants to encourage using mockgen@1.4.1, it seems like the mocks have been generated using a pre-1.0 version of mockgen. Using "go run github.com/golang/mock/mockgen" as a go:generate command instead of just "mockgen" avoids the need to pre-install into the developer's $PATH and uses the go.mod-specified version Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Upgrade gostats dependency from 0.2.6 to 0.4.0 (envoyproxy#141) My interest is the UDP protocol support which appeared in gotstats 0.3.10 There's a breaking change as of https://github.com/lyft/gostats/releases/tag/v0.3.0 which is that gostats no longer publishes stats as expvars. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Upgrade radix (envoyproxy#137) Signed-off-by: Tong Cai <caitong93@gmail.com> * cache_impl_test.go: fix failing test with ipv6 (envoyproxy#144) A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Split redis-specific logic from generic key-value store logic (envoyproxy#142) This is a pure refactoring with no behavior changes. It's a step toward being able to add memcache as a backend (see envoyproxy#140). This PR moves RateLimitCache from the redis package to a new "limiter" package, along with code for time/jitter, local cache stats, and constructing cache keys. All that can be reused with memcache. After this PR, the redis package is imported in exactly two places: - in service_cmd/runner/runner.go to call redis.NewRateLimiterCacheImplFromSettings() - in service/ratelimit.go in ShouldRateLimit to identify if a recovered panic is a redis.RedisError. If so, a stat is incremented and the panic() propagation is ended and in favor of returning the error as a the function result. The PR also includes changes by goimports to test/service/ratelimit_test.go so that the difference between package name vs file path name is explicit instead of implicit. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * json handler: return full ratelimit service response as json (envoyproxy#148) Previously an HTTP POST to /json would only return an HTTP status code, not all the other details supported by grpc ratelimit responses. With this change an HTTP POST to /json receives the full proto3 response encoded as json by jsonpb. It seems unlikely that anyone would be parsing the text "over limit" from the HTTP body instead of just reading the 429 response code, but for anyone doing that this would be a breaking change. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Update goruntime to latest, 0.2.5. Add new config for watching changes in runtime config folder directly instead of the runtime root dir. (envoyproxy#151) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Drop support for legacy ratelimit.proto and upgrade to v3 rls.proto (envoyproxy#153) Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * Followups to v3 upgrade (envoyproxy#155) - Regenerate mocks based on new default protocol - Manually transform v2 messages to v3 messages - some of the fields were renamed thus json Marshal/Unmarshal does not work anymore - Added tests that verify conversion v2<->v3 works for headers fields - Update tests to use proto.Equal - simple assert.Equals might not work correctly for protobuf messages. Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * Introduce a Dockerfile for running integration tests (envoyproxy#156) This diff creates Dockerfile.integration for running integration tests with clearly-defined dependencies. Previously the dependencies of the integration tests were defined within the github actions config. The new "make docker_tests" target should work for any developer with Docker installed. Previously there was no single command that would run integration tests across platforms, which makes development and onboarding harder. Even copying the command from github actions wouldn't have worked before, since that command quietly assumed that redis was already running on port 6379. Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Add support for rate limit overrides. (envoyproxy#158) Fixes envoyproxy#154 Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org> * redis client: default to use explicit pipelining (envoyproxy#163) Signed-off-by: Tong Cai <caitong93@gmail.com> * Clean go.mod file and update logrus to latest (envoyproxy#166) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Add full test environment example. Fix bug in existing docker-compose. (envoyproxy#170) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Implement LOG_FORMAT=json (envoyproxy#173) Centralized log collection system works better with logs in json format. E.g. DataDog strongly encourage setting up your logging library to produce your logs in JSON format to avoid the need for custom parsing rules. So, the next small fix is all we need to get json logs. Signed-off-by: Sergey Belyaev <sbelyaev@setronica.com> * ci: Update github action to push docker image tagged with sha for each merge to master branch (envoyproxy#176) Updates the github action to also push a tagged image based upon the git sha. The tag also includes the current version of the release. Example tag: envoyproxy/ratelimit:f1758150b6dfed3e5c0ae13fb7bb6b8f6ae00b0e Fixes envoyproxy#174 Signed-off-by: Steve Sloka <slokas@vmware.com> * Update README.md to refer to existing files (envoyproxy#178) Signed-off-by: Margaret Gorguissian <margaret.gorguissian@tufts.edu> * Add redis cluster and sentinel support (envoyproxy#179) Signed-off-by: Diego Erdody <diego@medallia.com> * Add support for x-ratelimit-reset header (envoyproxy#182) Signed-off-by: Clara Andrew-Wani <candrewwani@gmail.com> * Create repokitteh.star (envoyproxy#187) Signed-off-by: Itay Donanhirsh <itay@bazoo.org> * refactor NearLimitRatio to environment variable (envoyproxy#186) Signed-off-by: zufardhiyaulhaq <zufardhiyaulhaq@gmail.com> * Fix flakey tests with DurationUntilReset. Update docker example to V3 config. (envoyproxy#192) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Separate Redis cache and driver implementation (envoyproxy#194) Signed-off-by: William Albertus Dembo <w.albertusd@gmail.com> * Set ratelimit filter to v3 api (envoyproxy#196) Signed-off-by: Yuki Sawa <yukisawa@gmail.com> * Add debug logging to indicate descriptor and limit (envoyproxy#197) Signed-off-by: Sasha Kulbii <okulbii@wayfair.com> * Implement BACKEND_TYPE=memcache as an alternative k/v store to redis (envoyproxy#172) MEMCACHE_HOST_PORT=host:port must be set with BACKEND_TYPE=memcache To minimize roundtrips when getting multiple keys, the memcache implementation does a GetMulti to fetch the existing rate limit usage and does increments asynchronously in background goroutines, since the memcache API doesn't offer multi-increment. Resolves envoyproxy#140 Signed-off-by: David Weitzman <dweitzman@pinterest.com> * Refactoring of duplicated code across backend types (envoyproxy#202) Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com> * Small typo fix in README (envoyproxy#204) Signed-off-by: cpaika <paika.christopher@gmail.com> * Circle CI (#1) * Add Circle Config * Touch to build * Move to expected path * Use Docker Command * Do it all here then * Env * Add Readme * Actual README * Add Docker Compose (#2) * VAULT-893 Fix docker-compose.yml * VAULT-893 Fix docker-compose.yml Co-authored-by: Marshall Jones <marshall@offby3.com> Co-authored-by: Jose Ulises Nino Rivera <junr03@users.noreply.github.com> Co-authored-by: Daniel Hochman <danielhochman@users.noreply.github.com> Co-authored-by: Martien Verbruggen <martien.verbruggen@gmail.com> Co-authored-by: Ben Pope <BenPope@users.noreply.github.com> Co-authored-by: Steve Sloka <steve@stevesloka.com> Co-authored-by: Matt Klein <mattklein123@gmail.com> Co-authored-by: Charlie Vieth <charlie.vieth@gmail.com> Co-authored-by: Adil Hafeez <ahafeez@lyft.com> Co-authored-by: Kartograf <kartogrof@gmail.com> Co-authored-by: repl-david-winiarski <33431229+repl-david-winiarski@users.noreply.github.com> Co-authored-by: Junchao Lyu <6963707+freedomljc@users.noreply.github.com> Co-authored-by: tangxinfa <tangxinfa@gmail.com> Co-authored-by: Steve Sloka <slokas@vmware.com> Co-authored-by: Matt Klein <mklein@lyft.com> Co-authored-by: dblackdblack <github@dhb.is> Co-authored-by: David Weitzman <dweitzman@pinterest.com> Co-authored-by: Tong Cai <caitong@caicloud.io> Co-authored-by: Yuki Sawa <yukisawa@gmail.com> Co-authored-by: Petr Pchelko <petrpchelko@gmail.com> Co-authored-by: Petr Pchelko <ppchelko@wikimedia.org> Co-authored-by: Sergey Belyaev <svdba@users.noreply.github.com> Co-authored-by: Margaret G <Margaret.Gorguissian@tufts.edu> Co-authored-by: Diego Erdody <erdody@gmail.com> Co-authored-by: Clara <candrewwani@gmail.com> Co-authored-by: Itay Donanhirsh <itay@bazoo.org> Co-authored-by: Zufar Dhiyaulhaq <zufardhiyaulhaq@gmail.com> Co-authored-by: William Albertus Dembo <w.albertusd@gmail.com> Co-authored-by: Alex Kulbii <jncneo@gmail.com> Co-authored-by: Kateryna Nezdolii <nezdolik@spotify.com> Co-authored-by: Christopher <paika.christopher@gmail.com>

Signed-off-by: Tong Cai <caitong93@gmail.com>

A newly-added test in envoyproxy#137 checks the exact text of an error message which seems to vary when the network is tcp4 vs tcp6. This change relaxes the assertion to look for "connection refused" in a panic without making assumptions about what an IP address looks like. Example failure: --- FAIL: TestNewClientImpl (0.00s) --- FAIL: TestNewClientImpl/connection_refused (0.00s) cache_impl_test.go:442: Error Trace: cache_impl_test.go:442 Error: func (assert.PanicTestFunc)(0x1724110) should panic with error message: "dial tcp 127.0.0.1:12345: connect: connection refused" Panic value: "dial tcp [::1]:12345: connect: connection refused" Panic stack: goroutine 27 [running]: The testify assert package doesn't seem to support inexact matching on error messages, so the code gets a bit uglier than before. Signed-off-by: David Weitzman <dweitzman@pinterest.com>

caitong93 force-pushed the upgrade-radix branch from 274d55d to 95f9fbe Compare May 20, 2020 08:03

mattklein123 self-assigned this May 20, 2020

mattklein123 requested changes May 20, 2020

View reviewed changes

caitong93 force-pushed the upgrade-radix branch from af7ac67 to 24060b6 Compare May 21, 2020 12:39

caitong93 commented May 21, 2020

View reviewed changes

freedomljc reviewed May 21, 2020

View reviewed changes

caitong93 force-pushed the upgrade-radix branch from 512da30 to a1bb326 Compare May 26, 2020 13:24

freedomljc reviewed May 27, 2020

View reviewed changes

caitong93 added 10 commits May 27, 2020 23:09

Upgrade radix.v2 to radix.v3 and refactor redis cache impl to use pip…

3e2049a

…eline Signed-off-by: Tong Cai <caitong93@gmail.com>

Go generate mocks

a532854

Signed-off-by: Tong Cai <caitong93@gmail.com>

Fix unit tests

d02118b

Signed-off-by: Tong Cai <caitong93@gmail.com>

Add benchmark for DoLimit method

030544b

Signed-off-by: Tong Cai <caitong93@gmail.com>

Fix typo and update README

373994a

Signed-off-by: Tong Cai <caitong93@gmail.com>

Add unit tests for redis client impl

44516ce

Signed-off-by: Tong Cai <caitong93@gmail.com>

Fix unit test, do not use 6379 for a fake redis server.

42a7b3e

Signed-off-by: Tong Cai <caitong93@gmail.com>

Update testify to use assert.PanicsWithError

9f32d70

Signed-off-by: Tong Cai <caitong93@gmail.com>

Check specific error in TestNewClientImpl

81b4266

Signed-off-by: Tong Cai <caitong93@gmail.com>

Add more cases in BenchmarkParallelDoLimit

b7dee4d

Signed-off-by: Tong Cai <caitong93@gmail.com>

caitong93 force-pushed the upgrade-radix branch from 82650b4 to b7dee4d Compare May 27, 2020 15:09

freedomljc reviewed May 28, 2020

View reviewed changes

Tong Cai and others added 2 commits May 28, 2020 11:42

Merge branch 'master' into upgrade-radix

44686e9

Fix merge

679fd0f

Signed-off-by: Tong Cai <caitong93@gmail.com>

mattklein123 approved these changes May 28, 2020

View reviewed changes

mattklein123 merged commit afda91c into envoyproxy:master May 28, 2020

dweitzman mentioned this pull request May 29, 2020

cache_impl_test.go: fix failing test with ipv6 #144

Merged

ysawa0 mentioned this pull request Jul 15, 2020

Switch default protocol to v3 #152

Open

caitong93 mentioned this pull request Aug 7, 2020

redis client: default to use explicit pipelining #163

Merged

ysawa0 mentioned this pull request Aug 7, 2020

Cpu performance degradation introduced by #137 #164

Closed

erdody pushed a commit to medallia/ratelimit that referenced this pull request Sep 24, 2020

Upgrade radix (envoyproxy#137)

265c596

Signed-off-by: Tong Cai <caitong93@gmail.com> Signed-off-by: Diego Erdody <diego@medallia.com>

timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024

Upgrade radix (envoyproxy#137)

baccd67

Signed-off-by: Tong Cai <caitong93@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade radix #137

Upgrade radix #137

caitong93 commented May 20, 2020

mattklein123 left a comment

mattklein123 May 20, 2020

caitong93 May 21, 2020

mattklein123 May 20, 2020

moderation May 23, 2020

caitong93 commented May 21, 2020

caitong93 May 21, 2020

freedomljc left a comment

freedomljc May 21, 2020

caitong93 May 25, 2020

freedomljc May 21, 2020

caitong93 May 25, 2020

freedomljc May 21, 2020

caitong93 May 25, 2020

freedomljc May 21, 2020

caitong93 May 25, 2020 •

edited

Loading

freedomljc May 21, 2020

caitong93 May 25, 2020

freedomljc May 21, 2020

freedomljc May 21, 2020

caitong93 commented May 26, 2020 •

edited

Loading

freedomljc left a comment

freedomljc May 27, 2020

caitong93 May 27, 2020 •

edited

Loading

freedomljc May 27, 2020

caitong93 May 27, 2020 •

edited

Loading

freedomljc May 27, 2020

caitong93 May 27, 2020

freedomljc May 28, 2020

caitong93 commented May 27, 2020

freedomljc left a comment •

edited

Loading

caitong93 commented May 28, 2020 •

edited

Loading

mattklein123 left a comment

Upgrade radix #137

Upgrade radix #137

Conversation

caitong93 commented May 20, 2020

mattklein123 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

caitong93 commented May 21, 2020

Choose a reason for hiding this comment

freedomljc 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

caitong93 May 25, 2020 • 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

caitong93 commented May 26, 2020 • edited Loading

freedomljc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitong93 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caitong93 May 27, 2020 • 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

caitong93 commented May 27, 2020

freedomljc left a comment • edited Loading

Choose a reason for hiding this comment

caitong93 commented May 28, 2020 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

caitong93 May 25, 2020 •

edited

Loading

caitong93 commented May 26, 2020 •

edited

Loading

caitong93 May 27, 2020 •

edited

Loading

caitong93 May 27, 2020 •

edited

Loading

freedomljc left a comment •

edited

Loading

caitong93 commented May 28, 2020 •

edited

Loading