[redis-sentinel] adding integration with SDK scaffolding/testing facilities. (again) #66
Conversation
truthbk
changed the title
|
@krasnoukhov Not sure what happened to Travis, but it was attempting to build the wrong PR. Anyway's, closed the old PR and opened this one instead. I'll make sure the CI is green, update the manifest, and merge it ASAP. After that, you'll be the man in charge of |
|
@truthbk Sounds good. What about this thing I brought up on previous PR? I can create a new PR for that once this one is merged: #5 (comment) I'm not sure I'll be able to follow this project completely due to a high noise ratio, so would you be so kind of mentioning me on incoming redis-sentinel issues so I'll be able to take care of them? Thank you |
truthbk
force-pushed
the
jaime/redis-sentinel
branch
2 times, most recently
from
0c7d25e to
22950b6
Compare
|
@krasnoukhov I added a very basic integration test with an actual redis_sentinel, etc. Could you please review and check if it all makes sense to you? This is now quite close to being mergeable (I might have a couple of questions for you). Thank you! NOTE: please ignore the hbase errors, we have to fix those. Thank you. |
redis_sentinel/check.py
Outdated
| pending = stats.get('pending-commands', stats.get('link-pending-commands')) | ||
| if pending is not None: | ||
| self.gauge( | ||
| 'redis.sentinel.pending_commands', |
There was a problem hiding this comment.
Do you think it would make more sense if this was called link_pending_commands instead? I think it would be beneficial to be consistent with latest redis-provided name link-pending-commands
There was a problem hiding this comment.
I actually hit that issue when testing, so I'm all for this change. I'll rename.
|
Hey @truthbk, looks good overall, I just left one comment. Let me know if I can help |
redis_sentinel/check.py
Outdated
| pending = stats.get('pending-commands', stats.get('link-pending-commands')) | ||
| if pending is not None: | ||
| self.gauge( | ||
| 'redis.sentinel.pending_commands', pending, |
redis_sentinel/check.py
Outdated
| pending = stats.get('pending-commands', stats.get('link-pending-commands')) | ||
| if pending is not None: | ||
| self.gauge( | ||
| 'redis.sentinel.pending_commands', pending, tags=['master'] + master_tags |
redis_sentinel/metadata.csv
Outdated
| redis.sentinel.last_ok_ping_latency,gauge,,second,,number of seconds since last OK ping,,redis_sentinel,last ok latency | ||
| redis.sentinel.ok_sentinels,gauge,,instance,,number of sentinels up and running,,redis_sentinel,ok sentinels | ||
| redis.sentinel.ok_slaves,gauge,,instance,,number of slaves up and running,,redis_sentinel,ok slaves | ||
| redis.sentinel.pending_commands,gauge,,command,,number of pending sentinel commands,,redis_sentinel,pending commands |
There was a problem hiding this comment.
I believe this should be changed to link_pending_commands as well
|
@krasnoukhov thanks for the review, and good eye.... We should probably refactor all that common code at some point, it's error-prone. Anyways, I'll leave it as it is for now. |
|
LGTM @truthbk 👍 |
…ng/testing facilities.
[redis-sentinel] bring up infrastructure. [redis-sentinel][test] basic integration tests working. Still a relatively weak effort, but at least we have the integration tests working. We will probably need some mocked tests for the multiple sentinel scenario, as the current infra just brings up a single sentinel.
truthbk
force-pushed
the
jaime/redis-sentinel
branch
from
bca0992 to
193dddd
Compare
|
@krasnoukhov one last 👍 before merging? I am labeling you as the maintainer for the check in the manifest. Is that still OK with you. Once you give me the OK, we're all game! 🙇 |
|
Absolutely, we'll make sure we keep you in the loop for anything |
|
Just verified this in production, everything seems to work as expected. Thank you |
What does this PR do?
Adds
redis-sentinelusing SDK testing/scaffolding.Motivation
Community interest in
redis-sentinel. (cc. @krasnoukhov)Testing Guidelines
An overview on testing
is available in our contribution guidelines.