[400] Adding redis cluster support #531

ashikjm · 2021-07-07T12:45:03Z

Added tests for cluster support
Introducing isCluster flag
Using redisc module to implement cluster support

All discussions happened in #467 and #481

- Added tests for cluster support

coveralls · 2021-07-07T12:47:58Z

Pull Request Test Coverage Report for Build 1180

42 of 59 (71.19%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.8%) to 91.244%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
exporter/exporter.go	7	10	70.0%
exporter/redis.go	31	45	68.89%

Files with Coverage Reduction	New Missed Lines	%
exporter/redis.go	1	77.01%

Totals
Change from base Build 1173:	-0.8%
Covered Lines:	1511
Relevant Lines:	1656

💛 - Coveralls

codecov · 2021-07-07T12:48:02Z

Codecov Report

Merging #531 (369d9bd) into master (ef05f86) will decrease coverage by 0.93%.
The diff coverage is 71.64%.

@@            Coverage Diff             @@
##           master     #531      +/-   ##
==========================================
- Coverage   88.11%   87.17%   -0.94%     
==========================================
  Files          16       16              
  Lines        1313     1365      +52     
==========================================
+ Hits         1157     1190      +33     
- Misses         93      104      +11     
- Partials       63       71       +8

Impacted Files	Coverage Δ
exporter/redis.go	`70.00% <56.75%> (-16.12%)`	⬇️
exporter/exporter.go	`91.32% <66.66%> (-0.64%)`	⬇️
exporter/keys.go	`79.19% <100.00%> (+0.42%)`	⬆️
exporter/lua.go	`100.00% <100.00%> (ø)`
exporter/pwd_file.go	`100.00% <100.00%> (ø)`

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 ef05f86...369d9bd. Read the comment docs.

ashikjm · 2021-07-07T14:00:24Z

@oliver006 I closed the other PR and pulled the changes here. Please review.

oliver006 · 2021-07-08T13:43:42Z

Thanks for cleaning this up! I'll try to review this by the end of this week.

exporter/exporter_test.go

exporter/redis.go

moish83 · 2021-07-14T13:26:10Z

@ashikjm, great job, thank you.
when do you think this will be merged?
Another question, how will the exposed metrics differentiate between shards? Will we get another Prometheus label for shard number or something like that?

oliver006 · 2021-07-22T05:09:10Z

exporter/exporter_test.go

 	if err != nil {
-		t.Errorf("couldn't setup redis for uri %s, err: %s ", uri, err)
-		return err
+		log.Printf("Dial failed: %v", err)


Can you keep this is as Errorf() please?

oliver006 · 2021-07-22T05:09:31Z

exporter/exporter_test.go

 	return nil
+


Remove the blank line plz.

oliver006 · 2021-07-22T05:09:43Z

exporter/exporter_test.go

@@ -145,6 +161,73 @@ func deleteKeysFromDB(t *testing.T, addr string) error {

 	c.Do("DEL", TestSetName)
 	c.Do("DEL", TestStreamName)
+


No blank line plz.

oliver006 · 2021-07-22T05:10:04Z

exporter/exporter_test.go

+}
+
+func setupDBKeysCluster(t *testing.T, uri string) error {
+


no blank line plz

oliver006 · 2021-07-22T05:13:03Z

exporter/exporter_test.go


 	if _, err := c.Do("SELECT", dbNumStr); err != nil {
 		log.Printf("setupDBKeys() - couldn't setup redis, err: %s ", err)
 		// not failing on this one - cluster doesn't allow for SELECT so we log and ignore the error
 	}

 	for _, key := range keys {
-		_, err = c.Do("SET", key, TestValue)
+		_, err := c.Do("SET", key, TestValue)


Good catch.
You could even change this to if _, err := c.Do("SET", key, TestValue); err != nil { to make sure the scoping is as tight as possible.

exporter/redis.go

…tions - Linting the code

exporter/keys.go

houstonheat · 2021-07-25T18:16:37Z

exporter/redis.go

@@ -52,6 +64,50 @@ func (e *Exporter) connectToRedis() (redis.Conn, error) {
 	return c, err
 }

+func (e *Exporter) connectToRedisCluster() (redis.Conn, error) {
+	uri := e.redisAddr
+	if strings.Contains(uri, "://") {


Since you have already copied this code into TestClusterKeyValuesAndSizes(), I propose to move the stage of determining the address into a separate method (e.g. getClusterAddr()). In this case, it can also be tested and reused during unit testing:

// redis.go func (e *Exporter) getClusterAddr() string { clusterAddr := e.redisAddr if strings.Contains(clusterAddr, "://") { ... } else { ... } return clusterAddr }

I'd suggest to get rid of the copy in TestClusterKeyValuesAndSizes() - we control what uri is being passed to the tests and we shouldn't have to sanitize the input there.

exporter/exporter.go

exporter/redis.go

Co-authored-by: Hamidreza Kalantari <hamidreza.kalantari@cafebazaar.ir>

… increase performance (oliver006#538) * Add a type check to only run GET on keys of type string * Add comment for reason why PFCOUNT is ran first and not STRLEN when checking string length

- Optimizing code

ashikjm · 2021-09-04T11:25:11Z

@oliver006 I had to rebase to push changes to this branch. It was refusing to accept the push saying the branch was behind. But rebase has pulled a lot of changes from the master branch. Do you know a easy way to clean this? Last time I had to abandon the whole commit and raise a new PR.

oliver006 · 2021-09-04T11:32:20Z

Hmm, not sure what's the easiest way to fix this. My normal flow for this is to first do a git fetch or git pull and then git rebase -i origin/master and then you replay your changes on top of master (and you also squash a few commits).

I thing you could just leave this as is. I'll squash merge the PR when it's done and then the extra commits will go away. I'll try to get this reviewed by the end of today.

houstonheat · 2021-09-04T12:36:21Z

git pull --rebase origin master for short :)

oliver006 · 2021-09-04T23:31:40Z

exporter/keys_test.go

+	if strings.Contains(uri, "://") {
+		url, _ := url.Parse(uri)
+		if url.Port() == "" {
+			uri = url.Host + ":6379"
+		} else {
+			uri = url.Host
+		}
+	} else {
+		if frags := strings.Split(uri, ":"); len(frags) != 2 {
+			uri = uri + ":6379"
+		}
+	}
+


I don't think you need all of this, you can expect a correctly formed Uri for the tests.

oliver006 · 2021-09-04T23:37:54Z

exporter/redis.go

@@ -28,13 +31,22 @@ func (e *Exporter) connectToRedis() (redis.Conn, error) {
 		options = append(options, redis.DialPassword(e.options.Password))
 	}

+	if e.options.PasswordMap[uri] != "" {


Don't pass uri if this is all you need it for, instead pass password. Move the password check into the caller (and check both, e.options.password and the map and then pass it to configureOptions).

You should use e.redisAddr for the password lookup from the map, in that way it's the exact same format as provided by the user and can match what they put in the password file. Makes it easier to keep those two consistent.

Also, change the function signature to func (e *Exporter) configureOptions(uri string) ...

oliver006 · 2021-09-04T23:39:12Z

exporter/redis.go

@@ -52,6 +64,50 @@ func (e *Exporter) connectToRedis() (redis.Conn, error) {
 	return c, err
 }

+func (e *Exporter) connectToRedisCluster() (redis.Conn, error) {
+	uri := e.redisAddr
+	if strings.Contains(uri, "://") {


I'd suggest to get rid of the copy in TestClusterKeyValuesAndSizes() - we control what uri is being passed to the tests and we shouldn't have to sanitize the input there.

oliver006 · 2021-09-04T23:44:37Z

exporter/redis.go

+	}
+	log.Debugf("Running refresh on cluster object")
+	if err := cluster.Refresh(); err != nil {
+		log.Debugf("Cluster refresh failed: %v", err)


oliver006 · 2021-09-04T23:44:46Z

exporter/redis.go

+	log.Debugf("Creating redis connection object")
+	conn, err := cluster.Dial()
+	if err != nil {
+		log.Debugf("Dial failed: %v", err)


oliver006 · 2021-09-04T23:44:53Z

exporter/redis.go

+
+	c, err := redisc.RetryConn(conn, 10, 100*time.Millisecond)
+	if err != nil {
+		log.Debugf("RetryConn failed: %v", err)


oliver006 · 2021-09-04T23:48:36Z

exporter/exporter_test.go

@@ -59,40 +60,57 @@ func getTestExporterWithOptions(opt Options) *Exporter {
 	return e
 }

-func setupDBKeys(t *testing.T, uri string) error {
-	c, err := redis.DialURL(uri)
+func createClusterClient(uri string) (redis.Conn, error) {


Why a repeat all the logic here and not use connectToRedisCluster() instead?

All you need to do is set redisAddr, do something like this:

e := Exporter{redisAddr: uri} c, err := e.connectToRedisCluster() ...

* more logging when loading password file * log error when conversion fails

oliver006 · 2021-10-07T12:06:17Z

I see you pushed up some new commits (and also some old ones), I'll try to review this in the next couple of days.

ashikjm · 2021-10-07T12:19:32Z

I see you pushed up some new commits (and also some old ones), I'll try to review this in the next couple of days.

I only rebase from the master before pushing git pull --rebase origin master. But then my commits get rejected saying 'Updates were rejected because the tip of your current branch is behind its remote counterpart.'. Then I have to git pull and then all these get pulled in.

oliver006 · 2021-10-14T00:58:58Z

This looks good now @ashikjm - thanks for your patience and persistence!

oliver006 · 2021-10-14T13:36:09Z

Released as v1.28.0

ashikjm · 2021-11-01T08:02:47Z

This looks good now @ashikjm - thanks for your patience and persistence!

Thank you for bearing with me @oliver006 :)

[400] Adding redis cluster support

00fe768

- Added tests for cluster support

ashikjm mentioned this pull request Jul 7, 2021

[400] Adding redis cluster support. #481

Closed

oliver006 reviewed Jul 9, 2021

View reviewed changes

exporter/exporter_test.go Outdated Show resolved Hide resolved

exporter/exporter_test.go Show resolved Hide resolved

exporter/redis.go Show resolved Hide resolved

ashikjm added 2 commits July 20, 2021 22:01

[400] Optimizing code

0374f93

[400] Fixing lint issues

17c20b2

oliver006 reviewed Jul 22, 2021

View reviewed changes

[400] Consolidating all options configuration to function configureOp…

0c62938

…tions - Linting the code

houstonheat suggested changes Jul 25, 2021

View reviewed changes

oliver006 and others added 12 commits September 4, 2021 16:30

fix: convert age and idle fields to timestamps (oliver006#534)

7b1adca

Co-authored-by: Hamidreza Kalantari <hamidreza.kalantari@cafebazaar.ir>

pin golangci-lint docker image (oliver006#539)

36ad561

Add condition to check if key is of type string before running GET to…

2255726

… increase performance (oliver006#538) * Add a type check to only run GET on keys of type string * Add comment for reason why PFCOUNT is ran first and not STRLEN when checking string length

Golang 1.17

7280984

mirror images to quay.io

52738cc

use docker_config_json

e85d01b

go install

69136a0

disable coveralls

75610d1

run tests on tag, create docker image after tests

1dd7f31

ugh, forgot this one

cec8dd6

[400] Adding defer for cluster client in scrapeRedisHost func

a5581f8

- Optimizing code

Merge branch 'master' into 400_cluster_support_tests

c266108

oliver006 reviewed Sep 4, 2021

View reviewed changes

Add some addtl logging (oliver006#547)

b59e4d8

* more logging when loading password file * log error when conversion fails

rgeyer and others added 3 commits October 6, 2021 17:13

Use $__rate_interval for rate queries (oliver006#549)

afbbd95

[400] Tweaking code as per code review

e44ad20

[400] Removing dead code

369d9bd

oliver006 merged commit 27ed694 into oliver006:master Oct 14, 2021

oliver006 mentioned this pull request Oct 14, 2021

Export key value/length does not work with Redis cluster. #400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[400] Adding redis cluster support #531

[400] Adding redis cluster support #531

ashikjm commented Jul 7, 2021 •

edited

Loading

coveralls commented Jul 7, 2021 •

edited

Loading

codecov bot commented Jul 7, 2021 •

edited

Loading

ashikjm commented Jul 7, 2021

oliver006 commented Jul 8, 2021

moish83 commented Jul 14, 2021

oliver006 Jul 22, 2021

oliver006 Jul 22, 2021

oliver006 Jul 22, 2021

oliver006 Jul 22, 2021

oliver006 Jul 22, 2021

houstonheat Jul 25, 2021 •

edited

Loading

oliver006 Sep 4, 2021

ashikjm commented Sep 4, 2021

oliver006 commented Sep 4, 2021

houstonheat commented Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 Sep 4, 2021

oliver006 commented Oct 7, 2021

ashikjm commented Oct 7, 2021

oliver006 commented Oct 14, 2021

oliver006 commented Oct 14, 2021

ashikjm commented Nov 1, 2021

		@@ -145,6 +161,73 @@ func deleteKeysFromDB(t *testing.T, addr string) error {

		c.Do("DEL", TestSetName)
		c.Do("DEL", TestStreamName)

[400] Adding redis cluster support #531

[400] Adding redis cluster support #531

Conversation

ashikjm commented Jul 7, 2021 • edited Loading

coveralls commented Jul 7, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1180

💛 - Coveralls

codecov bot commented Jul 7, 2021 • edited Loading

Codecov Report

ashikjm commented Jul 7, 2021

oliver006 commented Jul 8, 2021

moish83 commented Jul 14, 2021

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

houstonheat Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashikjm commented Sep 4, 2021

oliver006 commented Sep 4, 2021

houstonheat commented Sep 4, 2021

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

oliver006 commented Oct 7, 2021

ashikjm commented Oct 7, 2021

oliver006 commented Oct 14, 2021

oliver006 commented Oct 14, 2021

ashikjm commented Nov 1, 2021

ashikjm commented Jul 7, 2021 •

edited

Loading

coveralls commented Jul 7, 2021 •

edited

Loading

codecov bot commented Jul 7, 2021 •

edited

Loading

houstonheat Jul 25, 2021 •

edited

Loading