-
Notifications
You must be signed in to change notification settings - Fork 957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Hide replicas from CLUSTER
subcmds in managed mode
#4174
Conversation
Part of #4173 (see for context)
tests/dragonfly/cluster_test.py
Outdated
async def test_emulated_cluster_with_replicas(df_factory): | ||
master = df_factory.create(port=BASE_PORT) | ||
master = df_factory.create(port=BASE_PORT, admin_port=BASE_PORT + 1000) | ||
replicas = [df_factory.create(port=BASE_PORT + i, logtostdout=True) for i in range(1, 3)] | ||
|
||
df_factory.start_all([master, *replicas]) | ||
|
||
c_master = aioredis.Redis(port=master.port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use master.client()
and master.admin_client()
instead of explicit aioredis.Redis
. the latter handles the closing of the connections automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I had to make some changes to follow up on that, because the clients are configured slightly differently (utf8 decoding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you get it built in.
@chakaz yes, we should do the same for real cluster in future PRs. |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping out with this!
* chore: Hide replicas from `CLUSTER` subcmds in managed mode Part of #4173 (see for context) * server.client()
Part of #4173 (see for context)
This fixes:
CLUSTER NODES
CLUSTER SHARDS
CLUSTER SLOTS
But only for emulated cluster mode. @romange - should we also handle real cluster (in a future PR)?