Fix slave_info miss label values #383
Conversation
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
==========================================
- Coverage 78.94% 78.90% -0.05%
==========================================
Files 2 2
Lines 912 910 -2
==========================================
- Hits 720 718 -2
Misses 157 157
Partials 35 35
Continue to review full report at Codecov.
|
|
Good catch, thanks for the PR! Can you please add tests around this scenario so this won't happen again in the future? |
|
Adding @jtorkkel to the conversation to see if he has suggestions as to what a test could look like, he added the master_host|port labels in the first place. |
exporter_test.go
Outdated
| @@ -1325,6 +1325,14 @@ func TestClusterSlave(t *testing.T) { | |||
| t.Errorf("Did not find key [%s] \nbody: %s", want, body) | |||
| } | |||
| } | |||
| for _, unwanted := range []string{ | |||
There was a problem hiding this comment.
Can we invert this test and check for presence of expected values?
There was a problem hiding this comment.
Hi, did not fully understood the question. In order to create and run multimaster tests you need to have KeyDB installed or you needs to include mock "info replication" for multimaster test cases. Redis OSS does not support multimaster, only simgle master nad multiple replicas. Redis EE, support active-active which is similar but not sure if same fields exposed through info.
The following info replication mock could be used for multimaster. Master 0 and 1 are up, Master 2 down.
127.0.0.1:8881> info replication
# Replication
role:active-replica
Master 0:
master_host:127.0.0.1
master_port:8882
master_link_status:up
master_last_io_seconds_ago:6
master_sync_in_progress:0
slave_repl_offset:16291
Master 1:
master_host:127.0.0.1
master_port:8883
master_link_status:up
master_last_io_seconds_ago:6
master_sync_in_progress:0
slave_repl_offset:16513
Master 2:
master_host:127.0.0.1
master_port:8884
master_link_status:down
master_last_io_seconds_ago:-1
master_sync_in_progress:0
slave_repl_offset:12740
master_link_down_since_seconds:62
slave_priority:100
slave_read_only:0
connected_slaves:2
slave0:ip=127.0.0.1,port=8882,state=online,offset=24123,lag=1
slave1:ip=127.0.0.1,port=8883,state=online,offset=24123,lag=1
master_replid:b11b52dc9c93567be5c05b1bee23d2c73c694457
master_replid2:0000000000000000000000000000000000000000
master_repl_offset:24123
second_repl_offset:-1
repl_backlog_active:1
repl_backlog_size:1048576
repl_backlog_first_byte_offset:1
repl_backlog_histlen:24123
127.0.0.1:8881>
There was a problem hiding this comment.
In the current implementation, slaveInfo["master_host"] and slaveInfo["master_port"] are always empty.
There was a problem hiding this comment.
Seem that same variable used in slave showing master address and in multimaster other masters.
Removing "continue" seems to fix the issue based on my testing and not breaking multimaster.
OSS slave:
127.0.0.1:8882> info replication
# Replication
role:slave
master_host:127.0.0.1
master_port:8881
master_link_status:up
master_last_io_seconds_ago:8
master_sync_in_progress:0
slave_repl_offset:308
slave_priority:100
slave_read_only:1
connected_slaves:0
master_port:8881
| @@ -1325,9 +1326,13 @@ func TestClusterSlave(t *testing.T) { | |||
| t.Errorf("Did not find key [%s] \nbody: %s", want, body) | |||
| } | |||
| } | |||
| hostReg, _ := regexp.Compile(`master_host="([0,1]?\d{1,2}|2([0-4][0-9]|5[0-5]))(\.([0,1]?\d{1,2}|2([0-4][0-9]|5[0-5]))){3}"`) | |||
There was a problem hiding this comment.
regular expressions seem to work
|
Thanks! |
No description provided.