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: Add SHOW LOCALITY command. #39058

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Jul 23, 2019

Fixes #38438.

SHOW LOCALITY returns the locality of the current node.

Release note (sql change): Add a SHOW LOCALITY command in sql.

@rohany rohany requested review from solongordon, awoods187 and a team July 23, 2019 19:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany
Copy link
Contributor Author

rohany commented Jul 23, 2019

./cockroach start --listen-addr=localhost --insecure --locality=region=us-east1
------
➜  cockroach git:(show-locality) ✗ ./cockroach sql --insecure
# Welcome to the cockroach SQL interface.
# All statements must be terminated by a semicolon.
# To exit: CTRL + D.
#
# Server version: CockroachDB CCL v19.2.0-alpha.20190606-964-ge9c579a369-dirty (x86_64-apple-darwin18.6.0, built 2019/07/23 19:27:01, go1.12.5) (same version as client)
# Cluster ID: 1be30eaf-3f36-4ae6-a2e7-867e4fce7410
# Organization: Cockroach Labs - Production Testing
#
# Enter \? for a brief introduction.
#
root@:26257/defaultdb> show locality;
     locality
+-----------------+
  region=us-east1
(1 row)

Time: 419µs

root@:26257/defaultdb>

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

  • Rather than making this a query, I think it would better if locality was just a new variable like node_id is. See here:

    cockroach/pkg/sql/vars.go

    Lines 520 to 525 in cee171c

    // CockroachDB extension.
    `node_id`: {
    Get: func(evalCtx *extendedEvalContext) string {
    return fmt.Sprintf("%d", evalCtx.NodeID)
    },
    },
    The implementation should look very similar actually since I think locality is stored on the evalCtx.
  • What do we want this to show if there is no locality? Right now it's blank, which is maybe fine or maybe we would prefer "none" or something? I guess we can make a note to ask @awoods187 when he gets back.
  • For commits like this with user-facing changes, you should fill out the "Release note" part of the commit message. That will be used by the docs team when they generate release notes, like this: https://www.cockroachlabs.com/docs/releases/v19.2.0-alpha.20190701.html

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187 and @solongordon)

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @rohany, and @solongordon)


pkg/sql/vars.go, line 516 at r1 (raw file):

				return evalCtx.Locality.String()
			}
			return "No locality set."

I changed my mind, I think this is kind of weird. :) Let's just stick with the blank value.

Addresses cockroachdb#38438.

`SHOW LOCALITY` returns the locality of current node.

Release note (sql change): Add a `SHOW LOCALITY` command in sql.
Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @awoods187 and @rohany)

@rohany
Copy link
Contributor Author

rohany commented Jul 31, 2019

bors r+

craig bot pushed a commit that referenced this pull request Jul 31, 2019
39058: sql: Add SHOW LOCALITY command. r=rohany a=rohany

Addresses #38438.

`SHOW LOCALITY` returns the locality of the current node.

Release note (sql change): Add a `SHOW LOCALITY` command in sql.


Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig
Copy link
Contributor

craig bot commented Jul 31, 2019

Build succeeded

craig bot pushed a commit that referenced this pull request Dec 10, 2019
40258: Diagram changes for documentation r=ericharmeling a=ericharmeling

Added changes to `diagrams.go` for the following statements:
- `SHOW PARTITIONS`
- `SHOW LOCALITY` (this change is a little hacky due to the fact that `SHOW LOCALITY` is, in fact, not defined in the SQL grammar... see #39058)
- `SHOW CREATE TABLE AS`
- `SHOW COLUMNS` (`WITH COMMENT`)
- `SHOW DATABASES` (`WITH COMMENT`)

@mjibson Adding you as reviewer because you created this generator.

Release note (docs): none

43075: roachtest: update Hibernate and psycopg blacklists r=rafiss a=rafiss

Hibernate now has no expected failures with our dialect, and psycopg has
one fewer expected failure.

closes #42819
closes #42434 

Release note: None

Co-authored-by: Eric Harmeling <eric.harmeling@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/cli: allow users to query for locality
3 participants