Skip to content

Exclude aux fields from "cluster nodes" and "cluster replicas" output #12166

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 4 commits into from
May 23, 2023
Merged

Exclude aux fields from "cluster nodes" and "cluster replicas" output #12166

oranagra merged 4 commits into from
May 23, 2023

Conversation

@PingXie
Copy link
Contributor

@PingXie PingXie commented May 12, 2023 •
edited by oranagra
Loading

This commit excludes aux fields from the output of the cluster nodes and cluster replicas command.
Specifically shard-id, which was introduced in 7.2 RC1 (see #10536).
We may decide to re-introduce them in some form or another in the future, but not in v7.2.

This commit excludes aux fields from the default output
0b25d5a 
of the "cluster nodes" command. The "shard-id" and other
aux fields will no longer appear in the default output.
To access the aux fields, the caller must specify the
"verbose" flag with the "cluster nodes" command (e.g.,
"cluster nodes verbose"). This change provides users
with the flexibility to retrieve the aux fields explicitly
when needed.
@PingXie
Copy link
Contributor Author

PingXie commented May 12, 2023

@oranagra @madolson @yossigo PR is ready for your review. Thanks!

madolson
madolson reviewed May 12, 2023
View reviewed changes
madolson
madolson reviewed May 12, 2023
View reviewed changes
@madolson
Copy link
Contributor

madolson commented May 12, 2023

@yossigo Related to what we discussed about omitting the extra information so that SHARD ID and nodename don't introduce a user facing different in 7.2.

@madolson madolson added state:major-decision Requires core team consensus approval-needed Waiting for core team approval to be merged state:needs-doc-pr requires a PR to redis-doc repository labels May 12, 2023
Ping Xie added 2 commits May 12, 2023 17:42
This commit excludes aux fields from the default output
9dc3022 
of the "cluster nodes" command. The "shard-id" and other
aux fields will no longer appear in the default output.
To access the aux fields, the caller must specify the
"verbose" flag with the "cluster nodes" command (e.g.,
"cluster nodes verbose"). This change provides users
with the flexibility to retrieve the aux fields explicitly
when needed.
Fix the version information for the new optional argument for "cluster
1d31c5c 
nodes"
@oranagra
Copy link
Member

oranagra commented May 13, 2023

so that SHARD ID and nodename don't introduce a user facing different in 7.2

@madolson the top comment says they're still visible, and iiuc it also adds an input argument (which from compatibility and interface has even higher impact)

@PingXie
Copy link
Contributor Author

PingXie commented May 13, 2023

@madolson the top comment says they're still visible, and iiuc it also adds an input argument (which from compatibility and interface has even higher impact)

Thanks for the feedback, @oranagra. The existing behavior of CLUSTER NODES remains unchanged, so there shouldn't be any immediate compatibility issues to worry about. Also, since the VERBOSE flag doesn't even exist, it's highly unlikely that anyone is relying on it currently. The question that I think is worth a double click on is do we see VERBOSE being a long term solution for any future extension to CLUSTER NODES, including but not limited to aux fields .

@oranagra
Copy link
Member

oranagra commented May 14, 2023

that's exactly what worries me.
i'm not concerned that the additional fields will break any clients (even if they're there by default), i'm more concerned about what will happen if in the future we'll want to remove them (or the verbose argument), and that'll break something.

@PingXie
Copy link
Contributor Author

PingXie commented May 14, 2023

Yeah, future-proofing is the key. Here's my take on the current situation:

  1. Today, CLUSTER NODES isn't limited to admins only. Clients have become heavily dependent on it. Although it's preferable to move the ecosystem to CLUSTER SHARDS, that'd be a separate discussion.
  2. We've now added extensibility via aux fields into the "cluster metadata database", i.e., nodes.conf, which shares the exact format as the output of CLUSTER NODES (assuming this PR isn't present).
  3. There are valid use cases where admins need to efficiently retrieve the entire nodes.conf content in one network roundtrip.
  4. The concern driving this PR is that exposing these new aux fields (and potential future extensions) increases the appcompat risk due to 1).

The question to me is if, how, and when we can reconcile 3) and 4), granted that 4) is a must-have

If

IMO, we should have a mechanism that allows admins to retrieve the entire content of nodes.conf in one call.

How

If we're comfortable with extending CLUSTER NODES, we could introduce a binary flag like VERBOSE (or another suitable name), which toggles between the "classic" and "full" views. It seems like a reasonable solution to me, and this is what this PR is about.

Alternatively, we could introduce a new command, starting fresh and avoiding further modifications to CLUSTER NODES.

When

I understand the concern about appcompat risks, so I'm also open to the suggestion of hiding all aux fields in 7.2 and avoiding further changes to CLUSTER NODES. 8.0 might be a more suitable option, giving us more time to carefully think through the longevity of any proposals.

@madolson
Copy link
Contributor

madolson commented May 15, 2023

I understand the concern about appcompat risks, so I'm also open to the suggestion of hiding all aux fields in 7.2 and avoiding further changes to CLUSTER NODES. 8.0 might be a more suitable option, giving us more time to carefully think through the longevity of any proposals.

I'm okay with this.

@madolson madolson mentioned this pull request May 16, 2023
Merged
@PingXie PingXie changed the title Exclude aux fields from default "cluster nodes" output, retrievable with "verbose" flag Exclude aux fields from "cluster nodes" and "cluster replicas" output May 21, 2023
@PingXie
Copy link
Contributor Author

PingXie commented May 21, 2023

All aux fields removed from cluster replicas and cluster nodes output.

oranagra
oranagra approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this should now be mentioned in the 7.2 release notes (since it reverts something that was mentioned in RC1 release notes)?

@madolson madolson added the release-notes indication that this issue needs to be mentioned in the release notes label May 23, 2023
@oranagra oranagra merged commit 4c74dd9 into redis:unstable May 23, 2023
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request May 24, 2023
@enjoy-binbin
Add dummy CLUSTER SLAVES call tests to fix reply ci
aa18cd6 
In redis#12166, we removed a call to CLUSTER SLAVES, which
then caused reply-schemas ci to fail:
```
WARNING! The following commands were not hit at all:
  cluster|slaves
  ERROR! at least one command was not hit by the tests
```

Because we already have command output that cover CLUSTER REPLICAS
elsewhere, here we simply add some dummy tests to fix the ci.
@enjoy-binbin enjoy-binbin mentioned this pull request May 24, 2023
Merged
oranagra pushed a commit that referenced this pull request May 24, 2023
@enjoy-binbin
Add dummy CLUSTER SLAVES call tests to fix reply ci (#12224)
ec5721d 
In #12166, we removed a call to CLUSTER SLAVES, which
then caused reply-schemas ci to fail:
```
WARNING! The following commands were not hit at all:
  cluster|slaves
  ERROR! at least one command was not hit by the tests
```

Because we already have command output that cover CLUSTER REPLICAS
elsewhere, here we simply add some dummy tests to fix the ci.
@PingXie PingXie deleted the shard-id branch May 24, 2023 14:55
@oranagra oranagra mentioned this pull request Jul 2, 2023
Merged
@oranagra oranagra mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

+2 more reviewers

@soloestoy soloestoy soloestoy approved these changes

@madolson madolson madolson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository

Projects

Redis 7.2
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PingXie @madolson @oranagra @soloestoy