Skip to content
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

sql, cli: Add attrs and locality to the node command #37947

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BramGruneir
Copy link
Member

@BramGruneir BramGruneir commented May 30, 2019

This patch fixes a number of aspects about how we display node and store
attributes as well as adds node attributes and localities to the

  • The String method on attributes proto was incorrectly using a , to join the
    array. Attributes always use a : so that it can be parsed via the command
    line and this fixes that.
  • Also, the internal tables crdb_interal.gossip_nodes,
    crdb_internal.kv_node_status and crdb_internal.kv_store_status had the
    attributes listed as JSON. This is not useful as it won't match what's in the
    command line. This converts it to a string.
  • This also adds the node attributes and localities to the
    cockroach node status command. As it was missing and can be helpful.

Release note (cli change): Node attributes and localities have been added to
the cockroach node status command.

Prior to this patch, localities were turned into a JSON object (not an array),
which removed the order. This now uses the same string format that we use when
declaring the localities in the command line.

Release note: None
@BramGruneir BramGruneir requested a review from a team as a code owner May 30, 2019 20:53
@BramGruneir BramGruneir requested review from a team May 30, 2019 20:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@BramGruneir
Copy link
Member Author

This is a follow up to #37944. So please ignore the first commit and comment on it in that PR.

Again, this could be backported or not. And I don't know our policy on changing crdb_internal tables.

This patch fixes a number of aspects about how we display node and store
attributes as well as adds node attributes and localities to the

* The String method on attributes proto was incorrectly using a `,` to join the
array. Attributes always use a `:` so that it can be parsed via the command
line and this fixes that.
* Also, the internal tables `crdb_interal.gossip_nodes`,
`crdb_internal.kv_node_status` and `crdb_internal.kv_store_status` had the
attributes listed as JSON. This is not useful as it won't match what's in the
command line. This converts it to a string.
* This also adds the node attributes and localities to the
`cockroach node status` command. As it was missing and can be helpful.

Release note (cli change): Node attributes and localities have been added to
the `cockroach node status` command.
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

LGTM on the overall result on the CLI (I agree that the CLI commands should print out their result in a format compatible with the command line arguments).

However as to the mechanism, I kind of like the availability of structured data internally. I think it is possible to "flatten" the structured data as an intermediate step, instead of at the source.

Also the CLI example needs a test, you can use the Example_XXXX model in cli_test.go (I think there are zone config / attribute config tests there already).

Reviewed 2 of 2 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball and @BramGruneir)


pkg/cli/node.go, line 164 at r2 (raw file):

                 END AS is_available,
            ifnull(is_live, false),
            locality,

And here you could use the newly introduced built-in function to render the result.


pkg/sql/crdb_internal.go, line 2384 at r2 (raw file):

				tree.NewDString(n.Desc.Address.NetworkField),
				tree.NewDString(n.Desc.Address.AddressField),
				tree.NewDString(n.Desc.Attrs.String()),

Ditto my comments on #37944, I would use JSON here and introduce a built-in function separately.

@bobvawter
Copy link
Member

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants